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

Add client metrics for http and ws transports #344

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

echlebek
Copy link
Contributor

@echlebek echlebek commented Jan 21, 2025

Not ready for merge

I've opened this PR in order to spur some discussion on how metrics could be reported by opamp-go clients and servers without introducing a dependency on open-telemetry or another metrics library. The goal here is to let library users inspect opamp-go's internal client metrics.

This PR adds client-side metrics collection for WS and HTTP transports. Collected metrics are in a custom format that can be translated to other metrics definitions by the library user. A TODO at this point is to add some library routines that can translate these metrics into common formats. It's unclear to me exactly where that should happen at the moment.

Counters have been added for message transmission metadata, as well as a ringbuffer that collects message latencies as well as key message attributes.

Library users can pass a types.ClientMetrics object to StartSettings and then inspect the results periodically. The methods that the counters and ringbuffers expose are goroutine-safe. If the metrics object is not passed, a default one with a minimal buffer is created, so this change should non-breaking for library users.

Some implementation details have been modified in order to ease testing. Specifically, some internal types have been replaced with interfaces in senders and receivers so that they can be replaced with fakes in tests.

The use of *websocket.Conn has been replaced with internal.WebsocketConn in several places for this reason as well.

Finally, some definitions of the server have changed in order to reflect the need for the number of bytes transmitted to be known by the caller. While this is not necessary for client-side metrics collection, it will help facilitate a similar change to the server so that metrics can be collected there too.

If this change or a change like it is merged, then we could also implement this part of the spec, building on top of the internal collection: https://github.com/observIQ/opamp-spec/blob/808917457e973c251fd6c968bbb2b9dd38cb9f3d/specification.md#own-telemetry-reporting (thanks @rnishtala-sumo for the suggestion)

@echlebek echlebek requested a review from a team as a code owner January 21, 2025 20:07
@echlebek echlebek marked this pull request as draft January 21, 2025 20:07
Copy link

codecov bot commented Jan 21, 2025

Codecov Report

Attention: Patch coverage is 92.77108% with 12 lines in your changes missing coverage. Please review.

Project coverage is 78.88%. Comparing base (e6fac32) to head (a3c9ea9).

Files with missing lines Patch % Lines
client/internal/wsreceiver.go 82.35% 2 Missing and 1 partial ⚠️
internal/wsmessage.go 62.50% 3 Missing ⚠️
client/internal/httpsender.go 95.00% 2 Missing ⚠️
client/internal/wssender.go 92.00% 1 Missing and 1 partial ⚠️
server/httpconnection.go 0.00% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #344      +/-   ##
==========================================
+ Coverage   77.81%   78.88%   +1.07%     
==========================================
  Files          25       27       +2     
  Lines        2321     2468     +147     
==========================================
+ Hits         1806     1947     +141     
- Misses        408      413       +5     
- Partials      107      108       +1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

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.

1 participant