Skip to content

Conversation

@Anilm3
Copy link
Contributor

@Anilm3 Anilm3 commented Apr 10, 2024

Simple PR which introduces the following changes:

  • Expose numeric_tags through the API, although the new API functions refer to metrics for consistency with other tracers (e.g. PHP).
  • Allow setting internal tags through the API, this is necessary as dd-trace-cpp is used by other internal projects such as ASM.

@Anilm3 Anilm3 requested a review from dmehala April 10, 2024 10:56
@pr-commenter
Copy link

pr-commenter bot commented Apr 10, 2024

Benchmarks

Benchmark execution time: 2024-04-15 08:09:40

Comparing candidate commit 98fa63c in PR branch anilm3/internal_tags_and_metrics with baseline commit b553930 in branch main.

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

@Anilm3 Anilm3 requested a review from cataphract April 11, 2024 14:29
@Anilm3 Anilm3 marked this pull request as ready for review April 11, 2024 14:30
@dmehala
Copy link
Collaborator

dmehala commented Apr 11, 2024

I don't know much about metrics. Until then, is there a way to test/view metrics sent to the Agent in the Datadog UI? In other words, how did you test it and how can I?

@dgoffredo
Copy link
Contributor

dgoffredo commented Apr 11, 2024

"metrics" is a word used to describe floating point valued (double) span tags. Hence my choice of the name "numeric tag" instead of "metric."

They're sent as "metrics" (along with normal tags, called "meta") in the messagepack/protobuf message sent to the Agent and then to Intake: https://github.com/DataDog/datadog-agent/blob/74fae3f4afeb77720e6584cccbbe853b5c9d677d/pkg/proto/datadog/trace/span.proto#L53-L55

It has nothing to do with metrics. "Metrics" is a terrible name. You can imagine how, historically, someone might have imagined it would be used for metrics, but it is not.

It is an open question which tags belong in "meta" (strings) and which tags belong in "metrics." There is some disagreement, e.g. for http.status_code.

My opinion is that tags are strings, and "metrics" is a ill-conceived attempt at optimization, or an artifact of some constraint imposed by some library in the Agent or Intake.

There's my unsolicited opinion. :)

edit: Talk to Doug Hawkins for more background.

Comment on lines -54 to -56
if (!tags::is_internal(key)) {
tags.insert_or_assign(key, value);
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Do you want any user of the library to be able to set internal tags? Same question in span.cpp.

If Datadog has an internal client who wishes to have special access to the span representation, then consider adding an API specific to that use case, and document it as such.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not sure there is much value in forbidding internal tags, however I'm open to the idea of separating the *_tag and *_metric methods into *_internal_tag and *internal_metric if that makes it more palatable.

@Anilm3
Copy link
Contributor Author

Anilm3 commented Apr 12, 2024

I don't know much about metrics. Until then, is there a way to test/view metrics sent to the Agent in the Datadog UI? In other words, how did you test it and how can I?

This is what I used to test it:

#include <datadog/span_config.h>
#include <datadog/tracer.h>
#include <datadog/tracer_config.h>

int main() {
    datadog::tracing::TracerConfig config;
    config.service = "test-service";
    config.environment = "staging";

    const auto validated_config = datadog::tracing::finalize_config(config);
    if (!validated_config) {
        return 1;
    }

    datadog::tracing::Tracer tracer{*validated_config};
    datadog::tracing::SpanConfig options;

    options.name = "metric-span";
    datadog::tracing::Span parent = tracer.create_span(options);

    parent.set_metric("metric", 10.0);

    return 0;
}

You'll need to have the agent running with the relevant API key, but you should be able to see a span containing the metric:

Screenshot from 2024-04-12 09-26-57

@Anilm3
Copy link
Contributor Author

Anilm3 commented Apr 12, 2024

"metrics" is a word used to describe floating point valued (double) span tags. Hence my choice of the name "numeric tag" instead of "metric."

They're sent as "metrics" (along with normal tags, called "meta") in the messagepack/protobuf message sent to the Agent and then to Intake: https://github.com/DataDog/datadog-agent/blob/74fae3f4afeb77720e6584cccbbe853b5c9d677d/pkg/proto/datadog/trace/span.proto#L53-L55

It has nothing to do with metrics. "Metrics" is a terrible name. You can imagine how, historically, someone might have imagined it would be used for metrics, but it is not.

It is an open question which tags belong in "meta" (strings) and which tags belong in "metrics." There is some disagreement, e.g. for http.status_code.

My opinion is that tags are strings, and "metrics" is a ill-conceived attempt at optimization, or an artifact of some constraint imposed by some library in the Agent or Intake.

There's my unsolicited opinion. :)

edit: Talk to Doug Hawkins for more background.

The reason I'm introducing metrics is for internal use, i.e. appsec needs to be able to send metrics / numeric tags. Note that internal in this case doesn't imply that they're necessarily prefixed with _dd. The use of numeric_tags internally seems reasonable, however I used the term metric for the new methods as it's the common convention across other tracers.

Copy link
Collaborator

@dmehala dmehala left a comment

Choose a reason for hiding this comment

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

I do agree "metrics" are essentially just tags. I also think *_numeric_tags is better suited but I also understand consistency across tracers is key. Thanks @dgoffredo to chiming in and sharing your knowledge 🙂

@cataphract cataphract merged commit e4cde4e into main Apr 15, 2024
@cataphract cataphract deleted the anilm3/internal_tags_and_metrics branch April 15, 2024 10:03
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.

5 participants