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

[telemetry] Add ability to set service.name for spans emitted by the Collector #10490

Merged
merged 4 commits into from
Jul 11, 2024

Conversation

jpkrohling
Copy link
Member

@jpkrohling jpkrohling commented Jul 1, 2024

Description

This PR bridges the config that is exposed to Collector users to the internal format expected by the config helpers around the tracer provider.

Link to tracking issue

Fixes #10489

Testing

Manual testing performed.

Config:

receivers:
  otlp:
    protocols:
      http:
      grpc:

exporters:
  debug:

service:
  pipelines:
    traces:
      receivers: [otlp]
      exporters: [debug]
    metrics:
      receivers: [otlp]
      exporters: [debug]
    logs:
      receivers: [otlp]
      exporters: [debug]
  telemetry:
    traces:
      processors:
        - batch:
            schedule_delay: 1000
            exporter:
              otlp:
                endpoint: https://otlp-gateway-prod-eu-west-2.grafana.net/otlp/v1/traces
                protocol: http/protobuf
                headers:
                  Authorization: "Basic ..."
    metrics:
      level: detailed
      readers:
        - periodic:
            exporter:
              otlp:
                endpoint: https://otlp-gateway-prod-eu-west-2.grafana.net/otlp/v1/metrics
                protocol: http/protobuf
                headers:
                  Authorization: "Basic ..."
    resource:
      "service.name": "otelcol-own-telemetry"

Sent this telemetry to the collector:

telemetrygen traces --traces 1 --otlp-insecure --otlp-attributes='cookbook="own-telemetry"'

Resulting in this:

image

Documentation

None.

Signed-off-by: Juraci Paixão Kröhling juraci@kroehling.de

@jpkrohling jpkrohling changed the title [telemetry] Map telemetry resource attributes to tracer provider [telemetry] Add ability to set service.name for spans emitted by the Collector Jul 1, 2024
@jpkrohling
Copy link
Member Author

@codeboten, this is in draft for two reasons:

  1. I don't know if you had something like this in mind for this part of the code, or if there's a mismatch between the config's spec and what we expose to end-users
  2. it lacks unit tests, which I can gladly write once the point 1 is sorted out :-)

@jpkrohling jpkrohling requested a review from codeboten July 1, 2024 16:45
Copy link
Contributor

@codeboten codeboten left a comment

Choose a reason for hiding this comment

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

Thanks for taking this on @jpkrohling. Yes this is roughly what I had in mind. Note that in a later version of the config library, the service name is treated as any other attributes so the conversion between the collector Resource and the config package's Resource should be simpler.

@codeboten
Copy link
Contributor

Depending if this merges before or after #10545, there will be changes required

@jpkrohling
Copy link
Member Author

That one can come first, it's simpler and the changes required for this one have to be applied anyway either here or there.

@jpkrohling
Copy link
Member Author

I looked at the tests, and I'm not sure what can be done to test this: the only exporters available are console, OTLP, and Zipkin. While I could spin up an OTLP test server locally, I think it's more than what would be reasonable for this test. Can you think of another solution?

@jpkrohling jpkrohling marked this pull request as ready for review July 8, 2024 12:03
@jpkrohling jpkrohling requested review from a team and djaglowski July 8, 2024 12:03
@codeboten
Copy link
Contributor

I looked at the tests, and I'm not sure what can be done to test this: the only exporters available are console, OTLP, and Zipkin. While I could spin up an OTLP test server locally, I think it's more than what would be reasonable for this test. Can you think of another solution?

If you've manually tested the change, i would suggest opening a follow up issue to add service test w/ mock OTLP receivers, as I think we should at least ensure that the data remains consistent as we make future changes. Something like https://github.com/open-telemetry/opentelemetry-collector/blob/main/exporter/otlpexporter/otlp_test.go might be a helpful inspiration

@jpkrohling
Copy link
Member Author

Created: #10596

Signed-off-by: Juraci Paixão Kröhling <juraci@kroehling.de>
Signed-off-by: Juraci Paixão Kröhling <juraci@kroehling.de>
Signed-off-by: Juraci Paixão Kröhling <juraci@kroehling.de>
Signed-off-by: Juraci Paixão Kröhling <juraci@kroehling.de>
Copy link

codecov bot commented Jul 11, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 92.34%. Comparing base (30de685) to head (1284459).

Additional details and impacted files
@@           Coverage Diff           @@
##             main   #10490   +/-   ##
=======================================
  Coverage   92.34%   92.34%           
=======================================
  Files         395      395           
  Lines       18686    18698   +12     
=======================================
+ Hits        17255    17267   +12     
  Misses       1070     1070           
  Partials      361      361           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@yurishkuro
Copy link
Member

  telemetry:
    traces:
      processors:
        - batch:
            schedule_delay: 1000
            exporter:
              otlp:
                endpoint: https://otlp-gateway-prod-eu-west-2.grafana.net/otlp/v1/traces

@jpkrohling is it possible for telemetry to be received/processed recursively by the collector itself? And could it be the default behavior?

@jpkrohling
Copy link
Member Author

The default behavior does not export any spans, and yes, it is possible for the snake to eat its own tail. I remember seeing recommendations to users to NOT do that, as it would end up with endless loops. Whenever possible, I mention that the monitor should never be the subject under monitoring.

@codeboten codeboten merged commit 08b0be7 into open-telemetry:main Jul 11, 2024
50 checks passed
@github-actions github-actions bot added this to the next release milestone Jul 11, 2024
codeboten added a commit that referenced this pull request Jul 22, 2024
This isn't as good as emitting telemetry w/ the service but at least it
ensures resource attributes are set.

Follow up to
#10490 and
#10645

Signed-off-by: Alex Boten <223565+codeboten@users.noreply.github.com>
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.

[telemetry] Setting service.name on own telemetry has no effect for traces
3 participants