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/civisibility: api refactor and support for telemetry metrics #2963

Merged
merged 22 commits into from
Nov 8, 2024

Conversation

tonyredondo
Copy link
Member

@tonyredondo tonyredondo commented Nov 4, 2024

What does this PR do?

This PR contains a complete refactor of the test api to follow and adopt Go conventions, and a complete support of the CI Visibility Telemetry Metrics.

https://app.datadoghq.com/dashboard/d4a-6h6-i8q/test-visibility-libraries-telemetry?fromUser=true&refresh_mode=paused&tpl_var_language_name%5B0%5D=go&from_ts=1730971500000&to_ts=1730982300000&live=false

Motivation

We already have telemetry metrics for all other languages, Go was missing this feature.

Reviewer's Checklist

  • Changed code has unit tests for its functionality at or near 100% coverage.
  • System-Tests covering this feature have been added and enabled with the va.b.c-dev version tag.
  • There is a benchmark for any new code, or changes to existing code.
  • If this interacts with the agent in a new way, a system test has been added.
  • Add an appropriate team label so this PR gets put in the right place for the release notes.
  • Non-trivial go.mod changes, e.g. adding new modules, are reviewed by @DataDog/dd-trace-go-guild.

Unsure? Have a question? Request a review!

@pr-commenter
Copy link

pr-commenter bot commented Nov 4, 2024

Benchmarks

Benchmark execution time: 2024-11-07 16:03:46

Comparing candidate commit 9c8f9e7 in PR branch tony/civisibility-telemetry-metrics with baseline commit bebced4 in branch main.

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

scenario:BenchmarkHttpServeTrace-24

  • 🟥 execution_time [+384.682ns; +462.718ns] or [+2.452%; +2.950%]

scenario:BenchmarkSetTagString-24

  • 🟩 execution_time [-4.832ns; -2.628ns] or [-4.203%; -2.286%]

@tonyredondo tonyredondo force-pushed the tony/civisibility-telemetry-metrics branch 2 times, most recently from 1c510d5 to a423ec3 Compare November 7, 2024 10:01
@tonyredondo tonyredondo changed the title internal/civisibility: support for telemetry metrics internal/civisibility: api refactor and support for telemetry metrics Nov 7, 2024
@tonyredondo tonyredondo marked this pull request as ready for review November 7, 2024 13:13
@tonyredondo tonyredondo requested review from a team as code owners November 7, 2024 13:13
Copy link
Contributor

@eliottness eliottness left a comment

Choose a reason for hiding this comment

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

I reviewed ONLY the code in internal/telemetry. Good overall

Other than that I would put a BIG warning on the performance hit your product could take considering the ongoing rework of the telemetry lib is partly done because of that: you can have a look a the usage of telemetry.GlobalClient.Count() and telemetry.GlobalClient.Record() all over dd-trace-go to understand that we never really added telemetry metrics anywhere because of the performance impact these functions have.

internal/telemetry/telemetry.go Outdated Show resolved Hide resolved
internal/telemetry/client.go Outdated Show resolved Hide resolved
internal/telemetry/client.go Outdated Show resolved Hide resolved
@tonyredondo
Copy link
Member Author

I reviewed ONLY the code in internal/telemetry. Good overall

Other than that I would put a BIG warning on the performance hit your product could take considering the ongoing rework of the telemetry lib is partly done because of that: you can have a look a the usage of telemetry.GlobalClient.Count() and telemetry.GlobalClient.Record() all over dd-trace-go to understand that we never really added telemetry metrics anywhere because of the performance impact these functions have.

I understand, I think at test level visibility where we don't have a massive concurrency and very hot paths is acceptable at least for the preview and dogfooding of the library. But, for sure we are looking forward on that rework you mention and to migrate this implementation in the future.

@tonyredondo tonyredondo force-pushed the tony/civisibility-telemetry-metrics branch from 9c8f9e7 to f935ab7 Compare November 8, 2024 09:01
@tonyredondo tonyredondo merged commit 80a7dbf into main Nov 8, 2024
149 of 150 checks passed
@tonyredondo tonyredondo deleted the tony/civisibility-telemetry-metrics branch November 8, 2024 09:08
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.

4 participants