-
Notifications
You must be signed in to change notification settings - Fork 246
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
chore: use imported telemetry types, clean bandwith metrics #5674
base: develop
Are you sure you want to change the base?
Conversation
Jenkins BuildsClick to see older builds (40)
|
go.mod
Outdated
@@ -292,3 +293,5 @@ require ( | |||
modernc.org/sqlite v1.14.2-0.20211125151325-d4ed92c0a70f // indirect | |||
zombiezen.com/go/sqlite v0.8.0 // indirect | |||
) | |||
|
|||
replace github.com/status-im/dev-telemetry => github.com/vpavlin/telemetry v0.0.0-20240807100636-642385d153a3 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This needs to be fixed before merging! Depends on status-im/telemetry#37
fa53483
to
9a6a644
Compare
90026cd
to
0217643
Compare
0217643
to
54ed67f
Compare
Rebased, tests seem to be running/happy, dropping |
type ProtocolStatsMap map[protocol.ID]metrics.Stats | ||
|
||
type SentEnvelope struct { | ||
Envelope *wakuv2protocol.Envelope | ||
PublishMethod common.PublishMethod | ||
} | ||
|
||
type ErrorSendingEnvelope struct { | ||
Error error | ||
SentEnvelope SentEnvelope | ||
} | ||
|
||
type ITelemetryClient interface { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should these types live in status-im/telemetry?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good question, they probably could be. I kept them in status-go because they are used in processAndPushTelemetry
(https://github.com/status-im/status-go/pull/5674/files#diff-6a98f0c7655c1846990bd03c6d7c1feab2b2d96ae9cf14b3bcd3d5c3677af6ffR199) along types like v2protocol.Envelope
and some other defined in the client
.
I'd say we could keep it this way as the next step still is to move the whole client to telemetry
repo and that will require some more thoughts on the API and/or how to abstract it.
But also happy move it now if that is the preference
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nah was just curious! PR looks good already!
This PR switches from using
map
s for telemetry data to using types exported bytelemetry
the project. It also removes extra/separate telemetry client for Waku bandwith metricsImportant changes:
waku/telemetry.go
in favour of usingtelemetry/client.go
github.com/status-im/telemetry
fortelemetry/pkg/types
map
with explicit types/structs