Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(telemetry)_: send connection failure metric #5518

Merged
merged 1 commit into from
Aug 5, 2024

Conversation

adklempner
Copy link
Contributor

@adklempner adklempner commented Jul 13, 2024

A short summary which serves as a squashed-commit message.

A description to understand introduced changes without reading the code.

Important changes:

Related to status-im/telemetry#21

@status-im-auto
Copy link
Member

status-im-auto commented Jul 13, 2024

Jenkins Builds

Click to see older builds (10)
Commit #️⃣ Finished (UTC) Duration Platform Result
✔️ ce321af #1 2024-07-13 03:36:26 ~2 min tests-rpc 📄log
✔️ ce321af #1 2024-07-13 03:37:47 ~4 min ios 📦zip
✔️ ce321af #1 2024-07-13 03:37:54 ~4 min linux 📦zip
✔️ ce321af #1 2024-07-13 03:38:55 ~5 min android 📦aar
✔️ ce321af #1 2024-07-13 04:18:40 ~44 min tests 📄log
✔️ 570ef17 #2 2024-07-13 03:49:14 ~1 min android 📦aar
✔️ 570ef17 #2 2024-07-13 03:49:24 ~2 min tests-rpc 📄log
✔️ 570ef17 #2 2024-07-13 03:49:29 ~2 min linux 📦zip
✔️ 570ef17 #2 2024-07-13 03:50:07 ~2 min ios 📦zip
✔️ 570ef17 #2 2024-07-13 05:02:11 ~43 min tests 📄log
Commit #️⃣ Finished (UTC) Duration Platform Result
✔️ 261dfa8 #3 2024-08-02 01:52:38 ~2 min tests-rpc 📄log
✔️ 261dfa8 #3 2024-08-02 01:54:24 ~4 min linux 📦zip
✔️ 261dfa8 #3 2024-08-02 01:54:24 ~4 min ios 📦zip
✔️ 261dfa8 #3 2024-08-02 01:55:07 ~5 min android 📦aar
✖️ 261dfa8 #3 2024-08-02 02:13:28 ~23 min tests 📄log
✔️ a274a15 #4 2024-08-02 05:38:35 ~2 min android 📦aar
✔️ a274a15 #4 2024-08-02 05:38:44 ~2 min tests-rpc 📄log
✔️ a274a15 #4 2024-08-02 05:38:56 ~2 min linux 📦zip
✔️ a274a15 #4 2024-08-02 05:39:43 ~3 min ios 📦zip
✔️ a274a15 #4 2024-08-02 06:20:10 ~43 min tests 📄log

@adklempner adklempner marked this pull request as ready for review August 2, 2024 05:36
@@ -125,6 +123,9 @@ func (w *Waku) broadcast() {
}
}

// Wraps the publish function with rate limiter
fn = w.limiter.ThrottlePublishFn(w.ctx, fn)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@richard-ramos I moved the rate limit wrapper after the telemetry wrapper so that rate limiting doesn't get caught as a publish error

Comment on lines +384 to +385
body, _ := json.Marshal(postBody)
jsonRawMessage := json.RawMessage(body)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We marshal and unmarshal here 🤔. And then we marshal again here:

body, err := json.Marshal(c.telemetryRetryCache)

It would be nice to reduce these procedures. We could change Client.TelemetryData type to interface{} and pass the postBody pointer directly. And then just call json.Marshal on it.

I guess it can be done out of this PR ofc.

Comment on lines +107 to +108
nextIdLock sync.Mutex
nextId int
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a suggestion, out of this PR

Suggested change
nextIdLock sync.Mutex
nextId int
nextId atomic.Int32

@adklempner adklempner merged commit 39497c2 into develop Aug 5, 2024
10 checks passed
@adklempner adklempner deleted the feat/telemetry-conn-failure branch August 5, 2024 18:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants