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] custom user-agent (& expanding OTEL_EXPORTER_OTLP_* env var support) #94

Merged

Conversation

robbkidd
Copy link
Member

@robbkidd robbkidd commented Apr 24, 2023

Instead of the Go autoinst agent appearing to telemetry data receivers as only the version of grpc-go against which it was built, this adds an identifier to the User-Agent header via the dial options parameter support within otlptracegrpc.

To help troubleshooting data transmission, runtime operating system and CPU architecture is included in the user-agent within a parenthetical comment in accordance with the header's specification.†

This replaces the low-level build up of a gRPC client connection with a reuse of the OTel Go SDK's grpc trace client constructor. A happy byproduct of using the SDK's constructor is that all of the OTEL_EXPORTER_OTLP_* environment variables related to exporting traces ought to be supported now, obviating the need to maintain code here to retrieve, interpret, and handle them.

A unit test to catch the hard-coded releaseVersion was added to cover version mismatches until the build pipeline can be updated to determine the proper version automatically.

Updated the instrumentation fixture output expectations to include the telemetry.auto.version attribute.

And resolves the following issues recorded in the keyval repo:

TODO:

  • confirm and link to a few existing issues these changes may solve
  • tests. tests would be good.

RFC 7231 though more readable documentation is available in the MDN Web Docs about the User-Agent header.

@MikeGoldsmith
Copy link
Member

This looks great, thanks @robbkidd 👍🏻

@robbkidd robbkidd force-pushed the robb.telemetry-about-telemetry branch 2 times, most recently from ef1bf42 to 3e442b7 Compare April 24, 2023 23:35
@robbkidd robbkidd marked this pull request as ready for review April 24, 2023 23:39
@robbkidd robbkidd requested review from a team and MikeGoldsmith April 24, 2023 23:39
@edeNFed
Copy link
Contributor

edeNFed commented Apr 25, 2023

Looks great!

I think it is worth adding to this PR the modification of releaseVersion variable as part of the release flow.
This should be fairly simple to do using ldflags

@damemi
Copy link
Contributor

damemi commented Apr 25, 2023

I checked out your branch and ran make fixture-nethttp locally, the auto-instrumentation container appears to be failing to connect to the collector:

$ kubectl logs pod/sample-job-6xwd5 -c auto-instrumentation
{"level":"info","ts":1682422873.1658528,"caller":"cli/main.go:37","msg":"starting Go OpenTelemetry Agent ..."}
{"level":"info","ts":1682422873.1659956,"caller":"opentelemetry/controller.go:114","msg":"Establishing connection to OTLP receiver ..."}
{"level":"error","ts":1682422873.1662185,"caller":"opentelemetry/controller.go:120","msg":"unable to connect to OTLP endpoint","error":"failed to build resolver: passthrough: received empty target in Build()","stacktrace":"go.opentelemetry.io/auto/pkg/opentelemetry.NewController\n\t/app/pkg/opentelemetry/controller.go:120\nmain.main\n\t/app/cli/main.go:45\nruntime.main\n\t/usr/local/go/src/runtime/proc.go:250"}
{"level":"error","ts":1682422873.1662562,"caller":"cli/main.go:47","msg":"unable to create OpenTelemetry controller","error":"failed to build resolver: passthrough: received empty target in Build()","stacktrace":"main.main\n\t/app/cli/main.go:47\nruntime.main\n\t/usr/local/go/src/runtime/proc.go:250"}

This would be the reason for the failure. The offsets failure looks more like a network timeout flake

@robbkidd
Copy link
Member Author

I think it is worth adding to this PR the modification of releaseVersion variable as part of the release flow. This should be fairly simple to do using ldflags

I was only starting to venture into making changes to the build or release workflows. I'm game to add that, but I'm not quite clear on the source of truth for a particular build's version.

Is it the versions.yaml? I'm not familiar with what tooling uses that as a release config file.

In some of the pipelines I've worked with, we'll determine a build version based on an identifiably-a-release tag or the distance from one (somewhat like the Collector does in its Makefile). Is that a pattern this project would want to adopt?

For example, git describe --always --match "v[0-9]*" HEAD would produce a releaseVersion for this branch as v0.1.0-alpha-8-g3e442b7: currently sitting on commit 3e442b7 which is 8 commits ahead of the closest tag that matches what a version tag starts with v0.1.0-alpha.

@robbkidd
Copy link
Member Author

robbkidd commented Apr 25, 2023

Looks great!

😅 Y'all are OK with the creation of the unit test suite†? And that the CI run of unit tests was added to the existing generate job?

† … that has only one test that probably needs changing if the source of truth for a version is not the versions.yaml file

@robbkidd
Copy link
Member Author

robbkidd commented Apr 25, 2023

appears to be failing to connect to the collector

Delegating the OTLP env var interpretation to the SDK means the endpoints need to have the protocol scheme included in the URL. (I believe.) Trying that in ab37628.

Another option (which is Working On My Machine™) would be to set OTEL_EXPORTER_OTLP_INSECURE to true for any instrument container, but I'd expect that to be determined when parsing an http:// scheme from an endpoint config value.

Update:

Yup. Parsed from the ENDPOINT value. Without the scheme present, the SDK defaults to https://.

Horray! Now the failure is the resource attribute I added to the code, but not to the test expectations! 🎉

@robbkidd
Copy link
Member Author

robbkidd commented Apr 26, 2023

After reading the release docs that are currently in-progress (#98), I think this would work for release version in code:

  • update the Makefile to set a GO_LDFLAGS env var with the incoming BuildID/ReleaseVersion (in whatever package it will ultimately be set and referenced with)
  • the Makefile would determine that version with git describe --always --match "v[0-9]*" HEAD - this will produce a clean version v0.1.0-alpha when run while on a commit with a release version tag and produce an informative "this is not a release" version ID when run with committed modifications in dev and on branches

A release build triggered by the tagging event described in the proposed RELEASING process would have the "clean" version tag.

I'm happy to include the git-tag-based release version update:

  1. in this PR
  2. open a new PR with that update
  3. work with @MikeGoldsmith to make the change in the releasing docs PR

EDIT: I've opened an issue proposing option 2, emphasized above.

Instead of the Go autoinst agent appearing to telemetry data receivers
as only the version of grpc-go against which it was built, this adds an
identifier to the User-Agent header via the dial options parameter
support within otlptracegrpc.

To help troubleshooting data transmission, runtime operating system and
CPU architecture is included in the user-agent within a parenthetical
comment in accordance with the header's specification.[1]

This replaces the low-level build up of a gRPC client connection with a
reuse of the OTel Go SDK's grpc trace client constructor. A happy
byproduct of using the SDK's constructor is that all of the
OTEL_EXPORTER_OTLP_* environment variables related to exporting traces
ought to be supported now, obviating the need to maintain code here to
retrieve, interpret, and handle them.

[1] RFC 7231 https://www.rfc-editor.org/rfc/rfc7231#section-5.5.3
    though more readable documentation about this header spec is
    available at MDN Web Docs:
    https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/User-Agent
This is an expendable commit if folks don't like it.
Spec and therefore SDK require the URL scheme in ENDPOINT values.
This Go auto-inst adds its version to resource attributes now.
@robbkidd robbkidd force-pushed the robb.telemetry-about-telemetry branch from dcbd454 to 6c7f3d6 Compare April 26, 2023 16:21
@robbkidd
Copy link
Member Author

Rebased to latest main. If all passes, I believe this is ready for merge with the work to automate the release version lookup during build tracked in another issue.

@robbkidd robbkidd changed the title custom user-agent (& expanding OTEL_EXPORTER_OTLP_* env var support) [feat] custom user-agent (& expanding OTEL_EXPORTER_OTLP_* env var support) Apr 26, 2023
Copy link
Contributor

@MrAlias MrAlias left a comment

Choose a reason for hiding this comment

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

Can you add a version.go similar to this: https://github.com/open-telemetry/opentelemetry-go/blob/86f325839ca329997672fbae8b054e7b6181b4d2/version.go (with a version_test.go). Our release tooling will update versions found there.

Makefile Outdated Show resolved Hide resolved
Makefile Outdated Show resolved Hide resolved
test/e2e/gorillamux/traces.json Show resolved Hide resolved
@robbkidd
Copy link
Member Author

robbkidd commented Apr 26, 2023

Can you add a version.go similar to this (with a version_test.go). Our release tooling will update versions found there.

It does? Excellent. I was ready to start wiring up a releaseinfo package and all manner of LDFLAGS shenanigans (#107) to get version wired in.

The version declared here will get bumped when using the multimod
releaser utility.

The agent's controller uses this version to include in the User-Agent
header and in a resource attribute for outgoing telemetry.
Modeled after the test targets in OpenTelemetry Go.

Removes the "maybe use gotestsum if you've got it!" option.
@robbkidd robbkidd requested a review from MrAlias April 27, 2023 11:25
Copy link
Contributor

@MrAlias MrAlias left a comment

Choose a reason for hiding this comment

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

Overall looks good, just minor issues. Thanks for the contribution!

version.go Outdated Show resolved Hide resolved
Makefile Show resolved Hide resolved
@robbkidd
Copy link
Member Author

robbkidd commented Apr 27, 2023

In thinking through this morning about the version number reflected in code, one disadvantage of the multimod-managed hard-coded value is that receivers of telemetry cannot distinguish uses of release builds versus dev builds. That's one benefit of the version being determined at build time, usually with ldflags set to the output of git describe.

As a vendor receiving telemetry from thousands of independent customer sources, Honeycomb has found it incredibly useful to be able to see more nuanced version information in user agents.

Update: I'm not proposing that ☝🏻 as a blocker to this PR; this PR makes things better! I can open a new issue or PR to consider appending more to the version number when built on a non-release commit and/or from a dirty working copy.

robbkidd and others added 2 commits April 27, 2023 11:22
Always Be Testing.

Co-authored-by: Tyler Yahn <[email protected]>
@MrAlias
Copy link
Contributor

MrAlias commented Apr 27, 2023

@robbkidd can you sync with main so we can merge this?

@robbkidd
Copy link
Member Author

@MrAlias Certainly! Done.

@MrAlias MrAlias merged commit 5741371 into open-telemetry:main Apr 27, 2023
@TylerHelmuth TylerHelmuth mentioned this pull request Apr 27, 2023
1 task
@robbkidd robbkidd deleted the robb.telemetry-about-telemetry branch April 28, 2023 01:25
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.

Cannot use a secure OTLP endpoint
5 participants