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

[exporter/datadog]: updated remove sublayer stats calc and mutex #3378

Conversation

ericmustin
Copy link
Contributor

Description:

This PR is an update of #2478

It removes sublayer stats calculation of trace payloads from the datadogexporter, as these are now calculated on the datadog backend. This follows work done in the datadog-agent, here: DataDog/datadog-agent#7450.

Removing this computation will improve performance and is an incremental step toward enabling support of distributed sublayer metric calculations.

Originally we had planned to hold off on shipping this until some broader work around the exportable/stats package itself was complete, which would allow us to use more up to date code exported from the datadog-agent...but given some user feedback on CPU consumption, I feel it makes sense to ship this now and get the benefit.

Link to tracking Issue:

Testing: removed all sublayer calc references and tests from test suite

Documentation:

@ericmustin ericmustin requested a review from mx-psi as a code owner May 12, 2021 23:07
@ericmustin ericmustin requested a review from a team May 12, 2021 23:07
@ericmustin
Copy link
Contributor Author

cc @mx-psi , wdyt here? any strong opinions on why this can't get shipped ahead of the exportable work?

@mx-psi
Copy link
Member

mx-psi commented May 14, 2021

Makes sense to ship it now given the CPU concerns, can you fix the merge conflicts? For added context, this supersedes the work on open-telemetry/opentelemetry-collector#2254.

@ericmustin
Copy link
Contributor Author

@mx-psi resolved conflicts, txs!

@@ -58,7 +61,7 @@ func computeAPMStats(tracePayload *pb.TracePayload, calculator *sublayerCalculat
Weight: 1,
TopLevel: true,
}
statsRawBucket.HandleSpan(weightedSpan, tracePayload.Env, []string{versionAggregationTag}, sublayers)
statsRawBucket.HandleSpan(weightedSpan, tracePayload.Env, []string{versionAggregationTag}, emptySublayer)
Copy link
Member

Choose a reason for hiding this comment

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

Is it equivalent that we pass an empty sublayer here (and thus leave empty the statsRawBucket fields) to using the new payload format from the Datadog Agent?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

😞 This is a good point. Upon closer inspection, no, it is not equivalent. I think due to changes in RawBucket and StatsRawBucket we're pinned here until the exportables package can get updated. I think our payloads would not be handled properly. fml. I'm going to close this...again lol...and followup with the impacted customers and provide some workarounds involving lowering batching time. Ultimately I think we're going to continue encountering issues in Fargate and other resource constrained environments until we both reduce the size of the collector itself (since contrib is large), as well as the resource consumption.

@mx-psi mx-psi self-assigned this May 14, 2021
@ericmustin ericmustin closed this May 15, 2021
@mx-psi mx-psi deleted the datadog_updated_refactor_stats_payload branch May 15, 2021 22:13
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.

3 participants