OTLP Logs - remove depth from scope fields #3843
Conversation
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## main #3843 +/- ##
==========================================
+ Coverage 87.45% 87.52% +0.07%
==========================================
Files 280 280
Lines 10765 10763 -2
==========================================
+ Hits 9414 9420 +6
+ Misses 1351 1343 -8
|
| @@ -145,18 +146,40 @@ internal static OtlpLogs.LogRecord ToOtlpLog(this LogRecord logRecord, SdkLimitO | |||
| otlpLogRecord.Flags = (uint)logRecord.TraceFlags; | |||
| } | |||
|
|
|||
| int scopeDepth = -1; | |||
| logRecord.ForEachScope(ProcessScope, otlpLogRecord); | |||
|
|
|||
| void ProcessScope(LogRecordScope scope, OtlpLogs.LogRecord otlpLog) | |||
There was a problem hiding this comment.
nit:
-
This can be made
static void ProcessScope(LogRecordScope scope, OtlpLogs.LogRecord otlpLog)now that it isn't using a closure. On newer runtimes the delegate will be created the first time it is accessed and then reused. -
For best perf, make a static field for the delegate and pass it in to logRecord.ForEachScope. That will ensure for all runtimes it is reused and doesn't need an existence check.
| else | ||
| { | ||
| if (OtlpKeyValueTransformer.Instance.TryTransformTag(scopeItem, out var result, attributeValueLengthLimit)) |
There was a problem hiding this comment.
nit: Looks like you could combine these into a single else if
| // Note: It is possible that we allow users | ||
| // to override this exporter feature. So not blocking | ||
| // empty/{OriginalFormat} in the SDK itself. |
There was a problem hiding this comment.
Are you suggesting that in the future we might add an exporter option to override this behavior?
My preference would be to try and avoid this solution if it becomes a need in the future. Given we have ILogger specific notions into our LogRecord data model we're required to handle them somehow in our exporters. But, I'd prefer to avoid spilling ILogger specific notions into our OTLP exporter options.
There was a problem hiding this comment.
deduping is not going to be Ilogger specific. It'll be same for any source which itself prevent duplicates.
For Activity, we do not produce duplicates, so if we want to have that similar behavior, we need to expose something.
(but it could be in the SDK or Exporter...)
| * Log Exporter modified to no longer prefix scope-depth when exporting ILogger | ||
| scopes as attributes. Empty keys and {OriginalFormat} key will be ignored from | ||
| scopes. |
There was a problem hiding this comment.
I'm not opposed to this change, but can you give a little more background on why we're making this change?
There was a problem hiding this comment.
Main motivation is the duplication/de-duplication cost, and undesired modification of user attributes.
Explanation for each of the case:
-
With plain string, the key is null/empty, which is unallowed in OTel. But even if we were to use some magic key like "scope", it'll be duplicated, when user have nested scopes. The .NET official docs are being fixed already to no longer showing scopes with strings.
-
With {OriginalFormat} - this is an unintentional side effect in Ms.Ext.Logging. It makes sense for Log message, where there is a Format() function, which can be used to get the fully formatted string. There is nothing like that for Scopes. "{OriginalFormat}" key will be duplicated if user has nested scopes, and we do not want to pay for dedup efforts.
The current approach simply prefixed a numeric value (based on scope depth), to solve this problem. But overriding user provide attributes with something else is undesirable (and affects perf too).
The following example can show why this is bad:
using (logger.BeginScope(new List<KeyValuePair<string, object>>
{
new KeyValuePair<string, object>("userid", "cijo")
}))
using (logger.BeginScope(new List<KeyValuePair<string, object>>
{
new KeyValuePair<string, object>("transactionid", "abcd")
}))
{
logger.LogInformation("Hello from {food} {price}.", "artichoke", 3.99);
}
the above would have resulted in attributes ("[0]:userid", "cijo"), ("[1]:transactionid", "abcd") as per current code, instead of ("userid", "cijo"). ("transactionid", "abcd"), per this PR.
| // key is going to be if user simply | ||
| // passes a string as scope. | ||
| // To summarize this exporter only allows | ||
| // IReadOnlyList<KeyValuePair<string, object?>> |
There was a problem hiding this comment.
Just mention IEnumerable<KeyValuePair<string, object?>>?
IReadOnlyList<KeyValuePair<string, object?>> implements IEnumerable<KeyValuePair<string, object?>> anyway.
Fixes #.
Changes
Please provide a brief description of the changes here.
For significant contributions please make sure you have completed the following items:
CHANGELOG.mdupdated for non-trivial changes