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

metrics: send metrics to the otel collector endpoint when active #2155

Merged
merged 1 commit into from
Jan 8, 2024

Conversation

jsternberg
Copy link
Collaborator

@jsternberg jsternberg commented Dec 12, 2023

Introduce a meter provider to the buildx cli that will send metrics to
the otel-collector included in docker desktop if enabled.

This will send usage metrics to the desktop application but also send
metrics to a user-provided otlp receiver endpoint through the standard
environment variables.

This introduces a single metric which is the cli count for build and
bake along with the command name and a few additional attributes.

@jsternberg jsternberg changed the title tracing: send traces to the otel collector when enabled metrics: send metrics to the otel collector endpoint when active Jan 2, 2024
@jsternberg jsternberg force-pushed the otel-exporter branch 2 times, most recently from 3d2c722 to c1a1d48 Compare January 3, 2024 15:41
@jsternberg jsternberg marked this pull request as ready for review January 3, 2024 15:42
Copy link
Member

@tonistiigi tonistiigi left a comment

Choose a reason for hiding this comment

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

Please add docs for how to collect/view the data and something that lists all the metrics that buildx will export.

If this docker.cli.count is something that currently serves more like a test value and may change in the future where more collection points are added then it should be behind BUILDX_ opt-in env var until it is considered stable.

go.mod Outdated
@@ -51,6 +51,12 @@ require (
k8s.io/client-go v0.26.7
)

require (
Copy link
Member

Choose a reason for hiding this comment

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

This new block in here probably isn't necessary.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Let me fix this.

@jsternberg
Copy link
Collaborator Author

@tonistiigi at the current moment, the metrics can't be exported or seen through a user-defined means. I'll add some docs for the metrics to get that started as I think we should allow a way to expose the metrics to end-users, but CLI metrics are a bit different from service metrics that they're generally more ephemeral.

I can add it behind an environment variable, but in its current iteration, these metrics can only be consumed by our backend without recompiling the source. I don't know if that changes the requirement to add an experimental flag for this. My fear here is that this data is already going to a backend that also has an experimental flag and we'll be adding too many experimental flags and won't get any data.

But yes. This metric is primarily meant to be a test one so I can see the metrics are working and to avoid Go telling me that the value isn't used.

@jsternberg jsternberg force-pushed the otel-exporter branch 3 times, most recently from 99c3d25 to 98197e8 Compare January 4, 2024 17:06
Copy link
Member

@tonistiigi tonistiigi left a comment

Choose a reason for hiding this comment

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

Something is wrong with the e2e tests.

@@ -926,3 +940,29 @@ func maybeJSONArray(v string) []string {
}
return []string{v}
}

func recordVersionInfo(mp metric.MeterProvider, command string) {
if !isExperimental() {
Copy link
Member

Choose a reason for hiding this comment

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

nit: add extra comment in here saying that we are still in the process of testing/stabilizing these counters.

@jsternberg jsternberg force-pushed the otel-exporter branch 4 times, most recently from 77e8c35 to d2f0a45 Compare January 5, 2024 19:38
@jsternberg
Copy link
Collaborator Author

jsternberg commented Jan 5, 2024

End to end tests are functional now. I've split the vendor update into the PR here so that will have to merge first.

The issue seemed to stem from how unix sockets are handled in the OTEL library. The WithEndpoint() option isn't intended to have a scheme and the environment variable removes the scheme before passing the option. So I mimic that behavior now. At the same time, unix sockets don't work without the scheme being passed to the endpoint so now there's custom logic for keeping the unix:// when the scheme is unix.

I was also accidentally passing an empty endpoint entirely because the otel section existed but no endpoint had been configured. That's also been fixed now.

Introduce a meter provider to the buildx cli that will send metrics to
the otel-collector included in docker desktop if enabled.

This will send usage metrics to the desktop application but also send
metrics to a user-provided otlp receiver endpoint through the standard
environment variables.

This introduces a single metric which is the cli count for build and
bake along with the command name and a few additional attributes.

Signed-off-by: Jonathan A. Sternberg <[email protected]>
@tonistiigi tonistiigi merged commit 78adfc8 into docker:master Jan 8, 2024
61 checks passed
@jsternberg jsternberg deleted the otel-exporter branch January 9, 2024 13:45
@crazy-max crazy-max added this to the v0.13.0 milestone Jan 12, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants