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

[service] add logger provider configuration support #10544

Open
wants to merge 9 commits into
base: main
Choose a base branch
from

Conversation

codeboten
Copy link
Contributor

This allows us to use the otel-go/config package to support configuring external destinations for logs. I'm putting this in draft to gather community feedback on whether this is a desirable feature for the collector.

I used the following configuration with this PR to send data to an OTLP backend:

telemetry:
   logs:
     processors:
       - batch:
           exporter:
             otlp:
               protocol: http/protobuf
               endpoint: https://api.honeycomb.io:443
               headers:
                 "x-honeycomb-team": "${env:HONEYCOMB_API_KEY}"

This allowed me to see logs in my backend:

Screenshot 2024-07-04 at 1 49 04 PM

@frzifus
Copy link
Member

frzifus commented Jul 20, 2024

While I think the ability to directly configure the destination of collector logs in the telemetry section might be cool for small and simple configurations in certain environments where you can not simply pick up your looks using system or file logs.
But I feel it would be more intuitive if we could specify a specific pipeline for exporting the data.

Something like this:

exporter:
 otlp/honeycomb:
   protocol: http/protobuf
   endpoint: https://api.honeycomb.io:443
     headers:
      "x-honeycomb-team": "${env:HONEYCOMB_API_KEY}"
pipeline:
   telemetry:
     metrics:
       verbosity: detailed
       pipelines: [logs/collector]
     logs:
       verbosity: detailed
       pipelines: [logs/collector]
   logs/collector:
       receivers:  [telemetry/logs]
       processors: [batch]
       exporters:  [otlp/honeycomb]
   metrics/collector:
       receivers:  [telemetry/metrics]
       processors: [batch]
       exporters:  [otlp/honeycomb]
   traces/collector:
      ...

Compared to:

telemetry:
   logs:
     processors:
       - batch:
           exporter:
             otlp:
               protocol: http/protobuf
               endpoint: https://api.honeycomb.io:443
               headers:
                 "x-honeycomb-team": "${env:HONEYCOMB_API_KEY}"
   metrics:
     processors:
       - batch:
           exporter:
             otlp:
               protocol: http/protobuf
               endpoint: https://api.honeycomb.io:443
               headers:
                 "x-honeycomb-team": "${env:HONEYCOMB_API_KEY}"
   traces:
     ...

wdyt?

@codeboten
Copy link
Contributor Author

@frzifus there was a decision made some time ago (i'm struggling to find a link with this info) to not pipe the collector's own telemetry through itself, to avoid the scenario of the collector's own telemetry being interrupted by an overloaded collector. This is why I'm proposing supporting the logging provider directly.

Are you thinking of the scenario where you'd like to be able to apply some transformations to the logs?

Copy link
Contributor

github-actions bot commented Aug 6, 2024

This PR was marked stale due to lack of activity. It will be closed in 14 days.

@github-actions github-actions bot added the Stale label Aug 6, 2024
@codeboten codeboten removed the Stale label Aug 6, 2024
Copy link
Contributor

This PR was marked stale due to lack of activity. It will be closed in 14 days.

@github-actions github-actions bot added the Stale label Aug 21, 2024
@codeboten codeboten removed the Stale label Aug 23, 2024
Copy link
Contributor

This PR was marked stale due to lack of activity. It will be closed in 14 days.

@github-actions github-actions bot added the Stale label Sep 11, 2024
@codeboten codeboten force-pushed the codeboten/add-log-record-processor-config branch from d8f5ce0 to d0e3509 Compare September 19, 2024 18:21
@codeboten codeboten marked this pull request as ready for review September 19, 2024 18:30
@codeboten codeboten requested a review from a team as a code owner September 19, 2024 18:30
Copy link

codecov bot commented Sep 19, 2024

Codecov Report

Attention: Patch coverage is 15.78947% with 16 lines in your changes missing coverage. Please review.

Project coverage is 92.08%. Comparing base (8211777) to head (dc5df50).

Files with missing lines Patch % Lines
service/telemetry/logger.go 5.88% 15 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main   #10544      +/-   ##
==========================================
- Coverage   92.14%   92.08%   -0.06%     
==========================================
  Files         432      432              
  Lines       20293    20309      +16     
==========================================
+ Hits        18699    18702       +3     
- Misses       1230     1243      +13     
  Partials      364      364              

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

@codeboten codeboten force-pushed the codeboten/add-log-record-processor-config branch from 4d01082 to 3ad3a90 Compare September 19, 2024 18:57
@codeboten codeboten removed the Stale label Sep 19, 2024
@codeboten codeboten force-pushed the codeboten/add-log-record-processor-config branch from 44aa182 to 09a60e5 Compare October 3, 2024 20:03
@codeboten
Copy link
Contributor Author

@open-telemetry/collector-approvers this PR should be ready for review

return nil, err
}
logger = logger.WithOptions(zap.WrapCore(func(_ zapcore.Core) zapcore.Core {
return otelzap.NewCore("go.opentelemetry.io/collector/service/telemetry", otelzap.WithLoggerProvider(sdk.LoggerProvider()))
Copy link
Member

@mx-psi mx-psi Oct 4, 2024

Choose a reason for hiding this comment

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

If you reach this point, I believe we are ignoring many of the existing options in the service::telemetry::logs section (e.g. output paths for sure, I wonder about some of the other options we already support). I think we should fail if said options are set and force the user to pick whether they want to use the processors section or the other options.

This allows us to use the otel-go/config package to support configuring external destinations
for logs. I'm putting this in draft to gather community feedback on whether this is a desirable
feature for the collector.

Signed-off-by: Alex Boten <223565+codeboten@users.noreply.github.com>
Signed-off-by: Alex Boten <223565+codeboten@users.noreply.github.com>
Signed-off-by: Alex Boten <223565+codeboten@users.noreply.github.com>
Signed-off-by: Alex Boten <223565+codeboten@users.noreply.github.com>
Signed-off-by: Alex Boten <223565+codeboten@users.noreply.github.com>
Signed-off-by: Alex Boten <223565+codeboten@users.noreply.github.com>
Signed-off-by: Alex Boten <223565+codeboten@users.noreply.github.com>
Signed-off-by: Alex Boten <223565+codeboten@users.noreply.github.com>
Signed-off-by: Alex Boten <223565+codeboten@users.noreply.github.com>
@codeboten codeboten force-pushed the codeboten/add-log-record-processor-config branch from 766208b to dc5df50 Compare October 10, 2024 19:51
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.

4 participants