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

APMSP-1241 Directly import trace-agent stats code for client-side stats #2817

Merged
merged 29 commits into from
Oct 31, 2024

Conversation

ajgajg1134
Copy link
Contributor

@ajgajg1134 ajgajg1134 commented Aug 13, 2024

WIP: This PR should be updated to point to the released v0.58 version of the trace-agent when it is available and then should be ready for merge. However this code is very unlikely to change other than changing the go.mod version so it should be safe to review now with minimal disruption.

What does this PR do?

Begin using the trace-agent's Concentrator instead of our own (mostly copied) implementation from the trace-agent.

Unfortunately there's some additional changes here needed like updating to go 1.22 as that's the minimum supported version of the trace-agent (but this is aligned with dd-trace-go's published version support policy). Additionally the google.golang.org/api contrib needed to be updated to support the more recent versions required by the trace-agent. I believe these imports / upgrades are ok but if we need to target a smaller import and modularize some more of the trace-agent to reduce impact here just let me know!

Motivation

Some new features introduced in the trace-agent concentrator have been missed here over time and this brings back feature parity when using client side stats (e.g. peer tags aggregation, marking trace root spans). We will eventually be enabling client stats by default (as it's more efficient!) and this is a required step to ensure that we don't break important stats features.

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!

go.mod Outdated
google.golang.org/grpc v1.57.1
google.golang.org/protobuf v1.33.0
google.golang.org/api v0.169.0
google.golang.org/grpc v1.64.0

Choose a reason for hiding this comment

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

🟡 Library Vulnerability

google.golang.org/grpc → 1.64.0

Private tokens could appear in logs if context containing gRPC metadata is logged in github.com/grpc/grpc-go (...read more)

Impact

This issue represents a potential PII concern. If applications were printing or logging a context containing gRPC metadata, the affected versions will contain all the metadata, which may include private information.

Patches

The issue first appeared in 1.64.0 and is patched in 1.64.1 and 1.65.0

Workarounds

If using an affected version and upgrading is not possible, ensuring you do not log or print contexts will avoid the problem.

View in Datadog  Leave us feedback  Documentation

go.mod Outdated
google.golang.org/grpc v1.57.1
google.golang.org/protobuf v1.33.0
google.golang.org/api v0.169.0
google.golang.org/grpc v1.64.0

Choose a reason for hiding this comment

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

🟡 Library Vulnerability

google.golang.org/grpc → 1.64.0

Private tokens could appear in logs if context containing gRPC metadata is logged in github.com/grpc/grpc-go (...read more)

Impact

This issue represents a potential PII concern. If applications were printing or logging a context containing gRPC metadata, the affected versions will contain all the metadata, which may include private information.

Patches

The issue first appeared in 1.64.0 and is patched in 1.64.1 and 1.65.0

Workarounds

If using an affected version and upgrading is not possible, ensuring you do not log or print contexts will avoid the problem.

View in Datadog  Leave us feedback  Documentation

@pr-commenter
Copy link

pr-commenter bot commented Aug 13, 2024

Benchmarks

Benchmark execution time: 2024-10-31 09:31:26

Comparing candidate commit 9c2328e in PR branch andrew.glaude/spanConcentrator2 with baseline commit 0bd0a8c in branch main.

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

scenario:BenchmarkOTelApiWithCustomTags/datadog_otel_api-24

  • 🟩 execution_time [-197.789ns; -162.611ns] or [-4.095%; -3.367%]

scenario:BenchmarkOTelApiWithCustomTags/otel_api-24

  • 🟩 allocations [-1; -1] or [-2.273%; -2.273%]
  • 🟩 execution_time [-407.377ns; -351.423ns] or [-5.529%; -4.770%]

scenario:BenchmarkSetTagStringer-24

  • 🟥 execution_time [+4.686ns; +8.854ns] or [+3.371%; +6.370%]

scenario:BenchmarkStartSpanConcurrent-24

  • 🟩 execution_time [-2.405µs; -2.256µs] or [-31.744%; -29.774%]

golang.org/x/text v0.16.0 // indirect
golang.org/x/tools v0.23.0 // indirect
google.golang.org/genproto/googleapis/rpc v0.0.0-20240520151616-dc85e6b867a5 // indirect
google.golang.org/grpc v1.64.0 // indirect

Choose a reason for hiding this comment

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

🟡 Library Vulnerability

google.golang.org/grpc → 1.64.0

Private tokens could appear in logs if context containing gRPC metadata is logged in github.com/grpc/grpc-go (...read more)

Impact

This issue represents a potential PII concern. If applications were printing or logging a context containing gRPC metadata, the affected versions will contain all the metadata, which may include private information.

Patches

The issue first appeared in 1.64.0 and is patched in 1.64.1 and 1.65.0

Workarounds

If using an affected version and upgrading is not possible, ensuring you do not log or print contexts will avoid the problem.

View in Datadog  Leave us feedback  Documentation

golang.org/x/xerrors v0.0.0-20231012003039-104605ab7028 // indirect
google.golang.org/protobuf v1.33.0 // indirect
google.golang.org/genproto/googleapis/rpc v0.0.0-20240520151616-dc85e6b867a5 // indirect
google.golang.org/grpc v1.64.0 // indirect

Choose a reason for hiding this comment

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

🟡 Library Vulnerability

google.golang.org/grpc → 1.64.0

Private tokens could appear in logs if context containing gRPC metadata is logged in github.com/grpc/grpc-go (...read more)

Impact

This issue represents a potential PII concern. If applications were printing or logging a context containing gRPC metadata, the affected versions will contain all the metadata, which may include private information.

Patches

The issue first appeared in 1.64.0 and is patched in 1.64.1 and 1.65.0

Workarounds

If using an affected version and upgrading is not possible, ensuring you do not log or print contexts will avoid the problem.

View in Datadog  Leave us feedback  Documentation

@ajgajg1134 ajgajg1134 force-pushed the andrew.glaude/spanConcentrator2 branch from ed5d824 to cfe668d Compare August 15, 2024 18:04
…agent

The dd-agent and google.golang.org/api both rely on google.golang.org/grpc, the old version
of the api package forces a downgrade of the datadog-agent which breaks the build.
@github-actions github-actions bot added the apm:ecosystem contrib/* related feature requests or bugs label Sep 4, 2024
@ajgajg1134 ajgajg1134 changed the title initial support for agent SpanConcentrator WIP APMSP-1241 Directly import trace-agent stats code for client-side stats [Waiting Agent Release] Sep 4, 2024
@ajgajg1134 ajgajg1134 marked this pull request as ready for review September 4, 2024 19:16
@ajgajg1134 ajgajg1134 requested a review from a team as a code owner September 4, 2024 19:16
@ajgajg1134 ajgajg1134 requested review from a team as code owners September 4, 2024 19:16
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.

Approving on behalf of appsec only for appsec code. Thanks for starting out the migration to rolling out go 1.21 👍

go.mod Outdated Show resolved Hide resolved
Copy link
Contributor

This PR is stale because it has been open 20 days with no activity. Remove stale label or comment or this will be closed in 10 days.

@github-actions github-actions bot added the stale Stuck for more than 1 month label Sep 26, 2024
@ajgajg1134 ajgajg1134 removed the stale Stuck for more than 1 month label Sep 26, 2024
@darccio
Copy link
Member

darccio commented Oct 9, 2024

@ajgajg1134 Was the agent released? When will this able to be merged?

@ajgajg1134
Copy link
Contributor Author

@ajgajg1134 Was the agent released? When will this able to be merged?

The current ETA for agent release is TOMORROW! 🎉

@ajgajg1134 ajgajg1134 changed the title APMSP-1241 Directly import trace-agent stats code for client-side stats [Waiting Agent Release] APMSP-1241 Directly import trace-agent stats code for client-side stats Oct 22, 2024
@darccio darccio enabled auto-merge (squash) October 31, 2024 10:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
apm:ecosystem contrib/* related feature requests or bugs team:apm-go
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants