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

[Logs] Revert logging API additions #3702

Merged

Conversation

@CodeBlanch CodeBlanch requested a review from a team as a code owner September 28, 2022 19:27
@codecov
Copy link

codecov bot commented Sep 28, 2022

Codecov Report

Merging #3702 (b77cf72) into main (4938c35) will decrease coverage by 0.68%.
The diff coverage is 90.00%.

❗ Current head b77cf72 differs from pull request most recent head da38f37. Consider uploading reports for the commit da38f37 to get more accurate results

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #3702      +/-   ##
==========================================
- Coverage   87.78%   87.10%   -0.69%     
==========================================
  Files         283      274       -9     
  Lines       10308    10008     -300     
==========================================
- Hits         9049     8717     -332     
- Misses       1259     1291      +32     
Impacted Files Coverage Δ
src/OpenTelemetry/Sdk.cs 100.00% <ø> (ø)
...lemetry/Logs/Options/OpenTelemetryLoggerOptions.cs 78.57% <80.00%> (-20.43%) ⬇️
...ryProtocol.Logs/OtlpLogExporterHelperExtensions.cs 92.85% <85.71%> (+1.19%) ⬆️
.../OpenTelemetry/Logs/OpenTelemetryLoggerProvider.cs 93.40% <100.00%> (-1.50%) ⬇️
...enTelemetry/Logs/OpenTelemetryLoggingExtensions.cs 100.00% <100.00%> (ø)
...entation/ExportClient/OtlpGrpcTraceExportClient.cs 35.71% <0.00%> (-42.86%) ⬇️
src/OpenTelemetry/Logs/LogRecordData.cs 58.82% <0.00%> (-41.18%) ⬇️
...xporter.OpenTelemetryProtocol/OtlpTraceExporter.cs 36.36% <0.00%> (-40.91%) ⬇️
...tation/OpenTelemetryProtocolExporterEventSource.cs 75.00% <0.00%> (-10.00%) ⬇️
...metryProtocol/Implementation/ActivityExtensions.cs 91.75% <0.00%> (-3.30%) ⬇️
... and 11 more

@alanwest
Copy link
Member

I'm beginning to suspect you of trying to outpace @cijothomas on lines added/removed with these giant diffs 🤣.

Screen Shot 2022-09-28 at 1 45 56 PM

@@ -222,16 +222,6 @@ Project("{9A19103F-16F7-4668-BE54-9A1E7A4F7556}") = "correlation", "docs\logs\co
EndProject
Project("{9A19103F-16F7-4668-BE54-9A1E7A4F7556}") = "OpenTelemetry.Tests.Stress.Logs", "test\OpenTelemetry.Tests.Stress.Logs\OpenTelemetry.Tests.Stress.Logs.csproj", "{4298057B-24E0-47B3-BB76-C17E81AF6B39}"
EndProject
Project("{9A19103F-16F7-4668-BE54-9A1E7A4F7556}") = "Examples.LoggingExtensions", "examples\LoggingExtensions\Examples.LoggingExtensions.csproj", "{F5EFF065-7AF5-4D7D-8038-CC419ABD8777}"
EndProject
Project("{9A19103F-16F7-4668-BE54-9A1E7A4F7556}") = "OpenTelemetry.Extensions.Serilog", "src\OpenTelemetry.Extensions.Serilog\OpenTelemetry.Extensions.Serilog.csproj", "{0D85558E-15B9-4251-BDBD-9CB7933B57E2}"
Copy link
Member

Choose a reason for hiding this comment

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

Why no more Serilog or EventSource extensions? Weren't they functional prototypes before, albeit with some InternalsVisibleTo hacks?

Copy link
Member Author

Choose a reason for hiding this comment

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

The way the "log appender" APIs work is they have to be fed the provider to use, or get it from DI. Originally that was OpenTelemetryLoggerProvider and then it became LoggerProvider. Basically they need some of the public API we are still sorting out.

Copy link
Member

Choose a reason for hiding this comment

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

Ok, makes sense. I'd lost track of what had been added and when.

Copy link
Member

Choose a reason for hiding this comment

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

This actually makes me more aware of the impact of the event api scope creep. Makes me question if the logging SIG would be amenable to first spec'ing and stabilizing a LoggerProvider/GetLogger/Emit suite of APIs that did not contain any notion of event domain/name. Seems like a smaller and more well understood problem space to just support the notion of log appenders.

Choose a reason for hiding this comment

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

Is the nuget package for OpenTelemetry.Extensions.Serilog still going to be available on myget.org, or is that going to disappear until a full solution is available?

Copy link
Member

Choose a reason for hiding this comment

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

all things removed from main will live in a separate branch and we'll do daily myget publish from that. (Similar to what was done for Metrics in early days.)

Copy link
Member Author

Choose a reason for hiding this comment

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

@pdonovan Working on that branch now it should be up soon with the changes 😄

@CodeBlanch
Copy link
Member Author

@alanwest

I'm beginning to suspect you of trying to outpace @cijothomas on lines added/removed with these giant diffs 🤣.

You better step up your game.

Copy link
Member

@alanwest alanwest left a comment

Choose a reason for hiding this comment

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

LGTM. It hurts 🤕 to see all this go away... but hopefully it will be a short-lived setback.

I can also take a pass over this diff after this PR merges just for an additional validation... core-1.3.0...main

@CodeBlanch CodeBlanch merged commit 7fd3783 into open-telemetry:main Sep 29, 2022
@CodeBlanch CodeBlanch deleted the logging-revert-api-additions branch September 29, 2022 00:18
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.

None yet

5 participants