-
Notifications
You must be signed in to change notification settings - Fork 772
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
[OTLP] Export LogRecord.CategoryName as InstrumentationScope name #4941
[OTLP] Export LogRecord.CategoryName as InstrumentationScope name #4941
Conversation
Codecov Report
@@ Coverage Diff @@
## main #4941 +/- ##
==========================================
- Coverage 83.65% 83.35% -0.30%
==========================================
Files 295 295
Lines 12325 12352 +27
==========================================
- Hits 10310 10296 -14
- Misses 2015 2056 +41
Flags with carried forward coverage won't be shown. Click here to find out more.
|
I have one bit of feedback. There is actually support for instrumentation scope on (I think this would make @alanwest happy because it would remove any knowledge of "category name" from OTLP 😄) |
That part requires experimental stuff. I would like to keep that one separate from this. |
Not true! Otlp will see it either public or internal but everything is there and the logic is the same either way. Whether or not the bridge api is available for other things to set it depends on experimental stuff but the SDK + LogRecord portion is always there. |
I was not aware of this: opentelemetry-dotnet/src/OpenTelemetry/AssemblyInfo.cs Lines 30 to 32 in 59f1d2c
I can do that as a separate change. If that is ok? |
src/OpenTelemetry.Exporter.OpenTelemetryProtocol/Implementation/OtlpLogRecordTransformer.cs
Outdated
Show resolved
Hide resolved
src/OpenTelemetry.Exporter.OpenTelemetryProtocol/Implementation/OtlpLogRecordTransformer.cs
Outdated
Show resolved
Hide resolved
src/OpenTelemetry.Exporter.OpenTelemetryProtocol/Implementation/OtlpLogRecordTransformer.cs
Outdated
Show resolved
Hide resolved
Works for me! |
src/OpenTelemetry.Exporter.OpenTelemetryProtocol/OtlpLogExporter.cs
Outdated
Show resolved
Hide resolved
scopeLogs.LogRecords.Add(otlpLogRecord); | ||
} | ||
} | ||
|
||
return request; | ||
} | ||
|
||
[MethodImpl(MethodImplOptions.AggressiveInlining)] | ||
internal void Return(OtlpCollector.ExportLogsServiceRequest request) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Stay consistent with Traces and Metrics and change this to an extension method?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Planning on doing same refactor for traces and metrics. will remove the static classes.
test/OpenTelemetry.Exporter.OpenTelemetryProtocol.Tests/OtlpLogExporterTests.cs
Outdated
Show resolved
Hide resolved
…https://github.com/vishweshbankwar/opentelemetry-dotnet into vibankwa/export-log-category-as-instrumentationscope
May fix #4867
Changes
This PR sets the InstrumentationScope
name
field under ScopeLogs toLogRecord.CategoryName
. With this change exported logs will be grouped by theLogRecord.CategoryName
.As an example consider following scenario
OTLP payload will look like below: The logs are grouped by CategoryName
Merge requirement checklist
CHANGELOG.md
files updated for non-trivial changes