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

internal/telemetry: collect telemetry info for gorilla/mux integration #1861

Merged
merged 33 commits into from
Apr 7, 2023

Conversation

lievan
Copy link
Contributor

@lievan lievan commented Apr 3, 2023

What does this PR do?

This PR implements collecting integration info for instrumentation telemetry. Currently it only tracks the gorilla/mux contrib package.

Unit testing for integration collection is done in contrib/internal/telemetrytest.

We also add a POC of a test (TestTelemetryEnabled) that verifies the expected contrib packages are importing telemetry.

Motivation

Collecting telemetry info for integrations will unlock a lot of useful data. In order to collect this data, we will have to add a call to telemetry.LoadIntegration inside an init function in every contrib package. This PR shows an example of how that might be done and tested in contrib/gorilla/mux. We can use this example to evaluate whether or not it is worth it to do this and maintain for every contrib package.

Describe how to test/QA your changes

There is currently a unit test in contrib/gorilla/mux, and we probably want to manual test with a sample app or with system tests as well. There is currently not a system test for integration collection.

Reviewer's Checklist

  • Changed code has unit tests for its functionality.
  • If this interacts with the agent in a new way, a system test has been added.

Sorry, something went wrong.

@lievan lievan added this to the v1.50.0 milestone Apr 3, 2023
@lievan lievan requested a review from a team as a code owner April 3, 2023 18:01
@lievan lievan requested a review from a team April 3, 2023 18:01
@pr-commenter
Copy link

pr-commenter bot commented Apr 3, 2023

Benchmarks

Comparing candidate commit 3fe7096 in PR branch evan.li/integration-telemetry with baseline commit ccb4c6a in branch main.

Found 1 performance improvements and 0 performance regressions! Performance is the same for 17 metrics, 0 unstable metrics.

scenario:BenchmarkConcurrentTracing-24

  • 🟩 execution_time [-0.198ms; -0.181ms] or [-14.404%; -13.152%]

lievan added 5 commits April 4, 2023 12:38

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature.

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature.
katiehockman
katiehockman previously approved these changes Apr 4, 2023
Copy link
Contributor

@katiehockman katiehockman left a comment

Choose a reason for hiding this comment

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

Yeah this is reasonable, and probably as minimally invasive as we're going to get. Thanks!! Just a few small comments.

katiehockman
katiehockman previously approved these changes Apr 5, 2023
@lievan lievan requested a review from katiehockman April 6, 2023 19:26
@lievan lievan enabled auto-merge (squash) April 7, 2023 19:04
@lievan lievan merged commit 4e267b4 into main Apr 7, 2023
@lievan lievan deleted the evan.li/integration-telemetry branch April 7, 2023 19:14
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.

None yet

2 participants