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

store logger in context, move LogsMessagesTrait into API #891

Closed
wants to merge 1 commit into from
Closed

store logger in context, move LogsMessagesTrait into API #891

wants to merge 1 commit into from

Conversation

brettmc
Copy link
Collaborator

@brettmc brettmc commented Dec 8, 2022

remove global/static LoggerHolder, and store logger in Context via Globals. LoggerHolder is deprecated, but a temporary BC hack should avoid immediate breakage.
Move LogsMessagesTrait into API, since there is a requirement (and some outstanding todo's) in the API layer to emit warnings in certain situations.

Closes #889

remove global/static LoggerHolder, and store logger in Context via Globals. LoggerHolder is deprecated, but a temporary
BC hack should avoid immediate breakage.
Move LogsMessagesTrait into API, since there is a requirement (and some outstanding todo's) in the API layer to emit
warnings in certain situations.
@brettmc brettmc requested a review from a team December 8, 2022 01:40
@codecov
Copy link

codecov bot commented Dec 8, 2022

Codecov Report

Merging #891 (f5f2cf1) into main (7bfc121) will decrease coverage by 0.02%.
The diff coverage is 89.47%.

Additional details and impacted files

Impacted file tree graph

@@             Coverage Diff              @@
##               main     #891      +/-   ##
============================================
- Coverage     81.59%   81.56%   -0.03%     
+ Complexity     2010     2008       -2     
============================================
  Files           264      264              
  Lines          5226     5230       +4     
============================================
+ Hits           4264     4266       +2     
- Misses          962      964       +2     
Flag Coverage Δ
7.4 81.04% <89.47%> (-0.03%) ⬇️
8.0 81.55% <89.47%> (-0.03%) ⬇️
8.1 81.69% <89.47%> (-0.03%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
src/Contrib/Grpc/GrpcTransportFactory.php 52.27% <ø> (ø)
src/Contrib/Jaeger/AgentExporter.php 100.00% <ø> (ø)
src/Contrib/Jaeger/HttpSender.php 100.00% <ø> (ø)
src/Contrib/Jaeger/JaegerTransport.php 86.66% <ø> (ø)
src/Contrib/Newrelic/Exporter.php 100.00% <ø> (ø)
src/Contrib/Otlp/MetricExporter.php 38.70% <ø> (ø)
src/Contrib/Otlp/SpanExporter.php 77.77% <ø> (ø)
src/Contrib/Otlp/SpanExporterFactory.php 100.00% <ø> (ø)
src/Contrib/Zipkin/Exporter.php 100.00% <ø> (ø)
src/Contrib/ZipkinToNewrelic/Exporter.php 85.00% <ø> (ø)
... and 15 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7bfc121...f5f2cf1. Read the comment docs.

@Nevay
Copy link
Contributor

Nevay commented Dec 8, 2022

I don't think we should make a PSR logger accessible via Globals, considering that the spec contains an experimental logging backend (also the reason why I dropped Globals::logger() from #822). IMO we should instead switch to using an otel Logger for our self-diagnostics once implemented.

@brettmc
Copy link
Collaborator Author

brettmc commented Dec 9, 2022

I don't think we should make a PSR logger accessible via Globals, considering that the spec contains an experimental logging backend (also the reason why I dropped Globals::logger() from #822). IMO we should instead switch to using an otel Logger for our self-diagnostics once implemented.

That makes sense, I'll close this one and do a simple migration from SDK to API.

@brettmc brettmc closed this Dec 9, 2022
@brettmc brettmc mentioned this pull request Dec 9, 2022
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.

make logging available from API
2 participants