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

Add metrics #450

Merged
merged 5 commits into from
May 31, 2024
Merged

Add metrics #450

merged 5 commits into from
May 31, 2024

Conversation

hoolioh
Copy link
Contributor

@hoolioh hoolioh commented May 24, 2024

What does this PR do?

Adds support for sending the following metrics:

  • trace_api.bytes
  • trace_chunks_sent
  • trace_chunks_dropped

Aside from the introduction of those new metrics a small refactor has been done on send_data.rs in order to address some technical debt from previous PRs. This changes can be summarized in:

  • Limit visibility of SendData fields.
  • Add default trait implementation for SendDataResult.
  • Add tests for both TraceHeaderTags and SendData.

Motivation

In order to turn the sidecar on by default for PHP there is the need to support those metrics.

@hoolioh hoolioh force-pushed the julio/APMSP-1004-add-metrics branch 6 times, most recently from e23a999 to f297af0 Compare May 24, 2024 13:54
@hoolioh hoolioh changed the title Julio/apmsp 1004 add metrics Add new metrics May 24, 2024
@hoolioh hoolioh changed the title Add new metrics Add metrics May 24, 2024
@codecov-commenter
Copy link

codecov-commenter commented May 24, 2024

Codecov Report

Attention: Patch coverage is 86.54867% with 76 lines in your changes are missing coverage. Please review.

Project coverage is 68.95%. Comparing base (ebe9a6a) to head (1a0aa63).

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #450      +/-   ##
==========================================
+ Coverage   68.55%   68.95%   +0.39%     
==========================================
  Files         193      193              
  Lines       25082    25569     +487     
==========================================
+ Hits        17195    17630     +435     
- Misses       7887     7939      +52     
Components Coverage Δ
crashtracker 19.34% <ø> (ø)
datadog-alloc 98.76% <ø> (ø)
data-pipeline 51.45% <ø> (ø)
data-pipeline-ffi 0.00% <ø> (ø)
ddcommon 85.16% <ø> (ø)
ddcommon-ffi 74.15% <ø> (ø)
ddtelemetry 56.09% <ø> (ø)
ipc 81.69% <ø> (ø)
profiling 77.97% <ø> (ø)
profiling-ffi 60.00% <ø> (ø)
serverless 0.00% <ø> (ø)
sidecar 37.33% <8.92%> (-0.43%) ⬇️
sidecar-ffi 0.00% <ø> (ø)
spawn-worker 54.98% <ø> (ø)
trace-mini-agent 69.12% <100.00%> (ø)
trace-normalization 97.79% <ø> (ø)
trace-obfuscation 95.74% <ø> (ø)
trace-protobuf 33.33% <ø> (+2.56%) ⬆️
trace-utils 92.25% <95.06%> (+2.24%) ⬆️

@hoolioh hoolioh force-pushed the julio/APMSP-1004-add-metrics branch from f297af0 to 512cdc5 Compare May 24, 2024 14:43
@hoolioh hoolioh marked this pull request as ready for review May 24, 2024 14:56
@hoolioh hoolioh requested review from a team as code owners May 24, 2024 14:56
Copy link
Contributor

@pierotibou pierotibou left a comment

Choose a reason for hiding this comment

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

Thanks a lot for doing this. TLDR, I assume we should comment the sending of the bytes metrics (we can keep the rest) as it's not yet supported. And add a few missing s to metric names.
The rest is mainly questions for my knowledge and some nitpicks that you should ignore :p

sidecar/src/self_telemetry.rs Show resolved Hide resolved
sidecar/src/self_telemetry.rs Outdated Show resolved Hide resolved
sidecar/src/self_telemetry.rs Outdated Show resolved Hide resolved
sidecar/src/self_telemetry.rs Outdated Show resolved Hide resolved
sidecar/src/self_telemetry.rs Outdated Show resolved Hide resolved
trace-utils/src/send_data.rs Outdated Show resolved Hide resolved
trace-utils/src/send_data.rs Show resolved Hide resolved
trace-utils/src/send_data.rs Show resolved Hide resolved
trace-utils/src/send_data.rs Outdated Show resolved Hide resolved
trace-utils/src/send_data.rs Outdated Show resolved Hide resolved
@morrisonlevi
Copy link
Contributor

As an experiment to get rid of String allocations in tags for static strings, I made a PR on top of this one: #453. We could take the same technique to the whole Vec<Tag> and do Cow<'static, [Tag]> or such to avoid the allocation of the vec as well. This is not a blocker for this PR, just maybe of interest to reviewers here.

@hoolioh hoolioh force-pushed the julio/APMSP-1004-add-metrics branch 2 times, most recently from 5bbddcb to dbb87cd Compare May 27, 2024 09:55
sidecar/src/self_telemetry.rs Outdated Show resolved Hide resolved
}
if trace_metrics.chunks_sent > 0 {
futures.push(self.send(
self.trace_chunk_sent,
Copy link
Contributor

Choose a reason for hiding this comment

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

I see that I'm late to the game...

@hoolioh hoolioh force-pushed the julio/APMSP-1004-add-metrics branch from dbb87cd to 711e6e2 Compare May 27, 2024 16:15
Copy link
Contributor

@pierotibou pierotibou left a comment

Choose a reason for hiding this comment

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

LGTM functionally (so please still wait for another review please).
Thanks a lot for the implementation and the follow up on all comments.

@hoolioh hoolioh force-pushed the julio/APMSP-1004-add-metrics branch from 711e6e2 to b34e8ce Compare May 29, 2024 10:26
sidecar/src/self_telemetry.rs Show resolved Hide resolved
trace-utils/src/trace_utils.rs Outdated Show resolved Hide resolved
sidecar/src/self_telemetry.rs Show resolved Hide resolved
@hoolioh hoolioh force-pushed the julio/APMSP-1004-add-metrics branch from b34e8ce to 19ca4c1 Compare May 30, 2024 12:55
trace-utils/src/send_data.rs Outdated Show resolved Hide resolved
trace-utils/src/send_data.rs Outdated Show resolved Hide resolved
trace-utils/src/send_data.rs Outdated Show resolved Hide resolved
trace-utils/src/send_data.rs Outdated Show resolved Hide resolved
hoolioh added 4 commits May 31, 2024 15:43
….bytes.

* Rework send_request method in order to return a meaningful enum in
  which pertinent information about metrics and response is held. This
  favours that all metrics are managed in the same place regardless if
  the trace sent either to the intake or through the agent.

* Limit the visibility for SendData fields in order to imporve their
  encapsulation.

* Add default trait implementation for SendData.

* Add support for the new metrics in the TraceFlusher service.
* Avoid sending tags for submitted_payloads, active_sessions and
  memory_usage.
* Comment code managing trace_api_bytes as there is no suppor it.
* Add status code to HTTP error logs.
* Define constants for for literals.
* Replace 'trace-chunk-*' for 'trace-chunks-*.
@hoolioh hoolioh force-pushed the julio/APMSP-1004-add-metrics branch from 0ed9d3d to 07475df Compare May 31, 2024 14:01
@hoolioh hoolioh force-pushed the julio/APMSP-1004-add-metrics branch from 07475df to 1a0aa63 Compare May 31, 2024 14:03
@hoolioh hoolioh merged commit 613f301 into main May 31, 2024
26 of 27 checks passed
@hoolioh hoolioh deleted the julio/APMSP-1004-add-metrics branch May 31, 2024 14:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants