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

Pass the missing async error channel into telemetry.Settings #12111

Merged
merged 6 commits into from
Jan 27, 2025

Conversation

omzmarlon
Copy link
Contributor

Description

Pass the missing async error channel into telemetry.Settings

Link to tracking issue

Fixes #11417

Testing

With the same setup as in #11417, building new otelcol with the changes in this PR, and running 2 instances with the same config using the same metric port, we would see proper crash error messages:

# config used:
receivers:
  nop:
exporters:
  nop:
service:
  pipelines:
    logs:
      receivers:
        - nop
      exporters:
        - nop
  telemetry:
    metrics:
      readers:
      - pull: 
          exporter:
            prometheus: 
              host: localhost
              port: 8889
# first instance log: 
./otelcol-custom --config otel-config.yaml
2025-01-16T17:36:34.638-0800    info    [email protected]/service.go:165 Setting up own telemetry...
2025-01-16T17:36:34.638-0800    info    telemetry/metrics.go:70 Serving metrics {"address": "localhost:8889", "metrics level": "Normal"}
2025-01-16T17:36:34.639-0800    info    [email protected]/service.go:231 Starting otelcol-custom...      {"Version": "", "NumCPU": 16}
2025-01-16T17:36:34.639-0800    info    extensions/extensions.go:39     Starting extensions...
2025-01-16T17:36:34.639-0800    info    [email protected]/service.go:254 Everything is ready. Begin running and processing data.
# second instance's log (using same config)
./otelcol-custom --config otel-config.yaml
2025-01-16T17:36:37.270-0800    info    [email protected]/service.go:165 Setting up own telemetry...
2025-01-16T17:36:37.270-0800    info    telemetry/metrics.go:70 Serving metrics {"address": "localhost:8889", "metrics level": "Normal"}
2025-01-16T17:36:37.271-0800    info    [email protected]/service.go:231 Starting otelcol-custom...      {"Version": "", "NumCPU": 16}
2025-01-16T17:36:37.271-0800    info    extensions/extensions.go:39     Starting extensions...
2025-01-16T17:36:37.271-0800    info    [email protected]/service.go:254 Everything is ready. Begin running and processing data.
2025-01-16T17:36:37.273-0800    error   [email protected]/collector.go:325       Asynchronous error received, terminating process    {"error": "listen tcp 127.0.0.1:8889: bind: address already in use"}
go.opentelemetry.io/collector/otelcol.(*Collector).Run
        go.opentelemetry.io/collector/[email protected]/collector.go:325
go.opentelemetry.io/collector/otelcol.NewCommand.func1
        go.opentelemetry.io/collector/[email protected]/command.go:36
github.com/spf13/cobra.(*Command).execute
        github.com/spf13/[email protected]/command.go:985
github.com/spf13/cobra.(*Command).ExecuteC
        github.com/spf13/[email protected]/command.go:1117
github.com/spf13/cobra.(*Command).Execute
        github.com/spf13/[email protected]/command.go:1041
main.runInteractive
        go.opentelemetry.io/collector/cmd/builder/main.go:49
main.run
        go.opentelemetry.io/collector/cmd/builder/main_others.go:10
main.main
        go.opentelemetry.io/collector/cmd/builder/main.go:42
runtime.main
        runtime/proc.go:272
2025-01-16T17:36:37.273-0800    info    [email protected]/service.go:296 Starting shutdown...
2025-01-16T17:36:37.274-0800    info    extensions/extensions.go:66     Stopping extensions...
2025-01-16T17:36:37.274-0800    info    [email protected]/service.go:310 Shutdown complete.

@omzmarlon omzmarlon requested a review from a team as a code owner January 17, 2025 01:40
@omzmarlon omzmarlon requested a review from djaglowski January 17, 2025 01:40
Copy link

linux-foundation-easycla bot commented Jan 17, 2025

CLA Signed

The committers listed above are authorized under a signed CLA.

@bogdandrutu
Copy link
Member

Please sign the CLA

@omzmarlon omzmarlon force-pushed the telemetry-async-err-ch branch from 7e837f2 to 393e350 Compare January 17, 2025 21:54
@atoulme
Copy link
Contributor

atoulme commented Jan 21, 2025

/easycla

Copy link

codecov bot commented Jan 21, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 91.62%. Comparing base (df99547) to head (afe82aa).
Report is 4 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main   #12111   +/-   ##
=======================================
  Coverage   91.62%   91.62%           
=======================================
  Files         465      465           
  Lines       24775    24776    +1     
=======================================
+ Hits        22699    22700    +1     
  Misses       1687     1687           
  Partials      389      389           

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

@omzmarlon omzmarlon force-pushed the telemetry-async-err-ch branch from 393e350 to 7ad37bc Compare January 21, 2025 22:29
@omzmarlon
Copy link
Contributor Author

Hi @bogdandrutu EasyCLA should be signed now. Should I add a changelog to this PR? Or make it skip changelog?

@atoulme
Copy link
Contributor

atoulme commented Jan 21, 2025

Please add a changelog - any test you can add to make sure this value is set helps as well. Thank you for your contribution.

To add a changelog in your checkout run: make chlog-new and follow instructions.

@omzmarlon
Copy link
Contributor Author

@bogdandrutu @atoulme added the change log. regarding unit test it's a bit hard to add because the functions that take telset do not have mocks so it's hard to meaningfully verify its content in test

@bogdandrutu bogdandrutu enabled auto-merge January 27, 2025 01:19
@bogdandrutu bogdandrutu added this pull request to the merge queue Jan 27, 2025
Merged via the queue into open-telemetry:main with commit 92c8136 Jan 27, 2025
52 checks passed
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.

Service Telemetry Prometheus metrics server does not properly report error
3 participants