[Logs] Fix: Respect AttributeValueLengthLimit when building otlp LogRecord data #3684

JWilh · 2022-09-21T06:43:18Z

Fixes avoid creating otlp data that is being rejected by monitoring tools, that have strict attribute length and attribute count restrictions.

Changes

Respect SdkConfiguration.AttributeValueLengthLimit and SdkConfiguration.AttributeCountLimit when adding attributes to the otlp LogRecord object.

For significant contributions please make sure you have completed the following items:

Appropriate CHANGELOG.md updated for non-trivial changes
Design discussion issue #
Changes in public API reviewed

linux-foundation-easycla · 2022-09-21T06:43:22Z

The committers listed above are authorized under a signed CLA.

✅ login: JWilh / name: Jonathan Wilhelm (0d2fe25)

utpilla · 2022-09-21T18:09:17Z

@JWilh Thanks for sending this PR! Could you please sign the CLA?

JWilh · 2022-09-28T09:39:47Z

@utpilla Sorry it took a little bit, I was lacking time to find the right person inside my company (right before code freeze and then I was some days off).
It's now properly signed.

JWilh · 2022-10-10T07:46:24Z

@utpilla Does it make sense to add this change to the ChangeLog and does it make sense to add the validation of OTEL_ATTRIBUTE_COUNT_LIMIT also in this pr?

…s are not rejected by monitoring tools

utpilla · 2022-10-10T17:12:08Z

@JWilh

Does it make sense to add this change to the ChangeLog

Yes, please update the CHANGELOG.

does it make sense to add the validation of OTEL_ATTRIBUTE_COUNT_LIMIT

You could do that in this one or a follow-up PR.

codecov · 2022-10-10T17:14:17Z

Codecov Report

Merging #3684 (0a503df) into main (73f8d3c) will increase coverage by 0.14%.
The diff coverage is 88.00%.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #3684      +/-   ##
==========================================
+ Coverage   87.21%   87.35%   +0.14%     
==========================================
  Files         277      277              
  Lines       10066    10068       +2     
==========================================
+ Hits         8779     8795      +16     
+ Misses       1287     1273      -14

Impacted Files	Coverage Δ
...etryProtocol/Implementation/LogRecordExtensions.cs	`85.88% <88.00%> (-2.07%)`	⬇️
...Telemetry/Internal/SelfDiagnosticsEventListener.cs	`96.87% <0.00%> (-0.79%)`	⬇️
...emetry.Api/Internal/OpenTelemetryApiEventSource.cs	`82.35% <0.00%> (+2.94%)`	⬆️
...metryProtocol/Implementation/ActivityExtensions.cs	`95.05% <0.00%> (+3.29%)`	⬆️
...tation/OpenTelemetryProtocolExporterEventSource.cs	`85.00% <0.00%> (+10.00%)`	⬆️
...xporter.OpenTelemetryProtocol/OtlpTraceExporter.cs	`77.27% <0.00%> (+18.18%)`	⬆️
...entation/ExportClient/OtlpGrpcTraceExportClient.cs	`78.57% <0.00%> (+28.57%)`	⬆️

JWilh · 2022-10-10T20:15:11Z

Windows test run failed with

[xUnit.net 00:00:48.91] OpenTelemetry.Instrumentation.AspNetCore.Tests.InProcServerTests.ExampleTest [FAIL]
[xUnit.net 00:00:48.91] System.Net.Http.HttpRequestException : The SSL connection could not be established, see inner exception.
[xUnit.net 00:00:48.92] ---- System.Security.Authentication.AuthenticationException : The remote certificate is invalid because of errors in the certificate chain: UntrustedRoot

Is this issue already know or just a "transient" error?

alanwest · 2022-10-10T21:05:45Z

src/OpenTelemetry.Exporter.OpenTelemetryProtocol/Implementation/LogRecordExtensions.cs

+                    otlpLogRecord.Attributes.AddStringAttribute(SemanticConventions.AttributeExceptionType, logRecord.Exception.GetType().Name, attributeValueLengthLimit);
+                    otlpLogRecord.Attributes.AddStringAttribute(SemanticConventions.AttributeExceptionMessage, logRecord.Exception.Message, attributeValueLengthLimit);
+                    otlpLogRecord.Attributes.AddStringAttribute(SemanticConventions.AttributeExceptionStacktrace, logRecord.Exception.ToInvariantString(), attributeValueLengthLimit);


I suggest we get rid of the AddStringAttribute method and use OtlpKeyValueTransformer.Instance.TryTransformTag even for these one-offs. That way we benefit from the uniform way we process string attributes.

Also, OTLP tag transform has a test suite, so the truncation logic is at least covered there. It'd be nice to also have some tests specifically for the log exporter logic. That said, we do not yet have tests for the trace exporter either with respect to attribute limits.

I didn't removed the AddStringAttribute function, but replaced the implementation to use the OtlpKeyValueTransformer.Instance.TryTransformTag.
Removing the function itself was for me to excessive (additional if for the TryTransform and for the AttributeCount).
Is that good for you?

About tests: I will check during the next days when I have a free timeslot. 👍

alanwest · 2022-10-10T21:07:31Z

Is this issue already know or just a "transient" error?

Not likely related to this PR. I assume it's a flaky test, but would need to investigate.

alanwest · 2022-10-10T21:08:03Z

Related open-telemetry/opentelemetry-specification#2861

JWilh · 2022-10-11T05:30:53Z

Related open-telemetry/opentelemetry-specification#2861

I like it, should I already add the new env var names for the log record limits or better stay with the current ones and they are changed later on in a new pr once the specification pr is merged?

=> Adding the new env vars in a later pr would not create a breaking change, as the configuration would fall back to the general attributelenght/count limit vars, if the LogRecords env vars are not defined, correct?

…gAttribute and in AddIntAttribute

alanwest · 2022-10-11T20:05:58Z

I like it, should I already add the new env var names for the log record limits or better stay with the current ones and they are changed later on in a new pr once the specification pr is merged?

Ok to do later. The name may be different in the end. Most likely either OTEL_LOGRECORD_* or OTEL_LOG_*.

alanwest

LGTM. Thanks for working on this @JWilh!

I'm not inclined to hold this PR up for tests for the log exporter logic as we don't currently have tests for the trace exporter either. But if you have some cycles to add some tests in a follow up that be greatly appreciated! ❤️

JWilh · 2022-10-12T14:42:59Z

Draft for unittest: #3758
The relevant commit is this one:e8a14c4

JWilh · 2022-10-12T14:44:47Z

@alanwest This pr here is merged by you or @utpilla, is this correct?
(as I don't have permissions to actually merge it)

alanwest · 2022-10-12T23:40:56Z

@alanwest This pr here is merged by you or @utpilla, is this correct?

Ah, yes, this is correct! Apologies for not elaborating... we normally wait for at least a couple sets of eyes on a PR before merging. I'll poke one of the other maintainers and see if we can get this merged soon.

src/OpenTelemetry.Exporter.OpenTelemetryProtocol/Implementation/LogRecordExtensions.cs

CodeBlanch · 2022-10-13T17:03:55Z

Merged so I can get this working with #3760

CodeBlanch · 2022-10-13T17:21:50Z

src/OpenTelemetry.Exporter.OpenTelemetryProtocol/Implementation/LogRecordExtensions.cs

        {
-            repeatedField.Add(new OtlpCommon.KeyValue
+            if (logRecord.Attributes.Count < maxAttributeCount)


@alanwest One random thought looking at this PR about how we could improve things...

We build the OtlpCommon.KeyValue before we check if we will drop it. Could probably improve that so we skip the allocation(s) if we know we are up against the limit?

@alanwest I just tried changing it for the LogRecord extension, but we would need something likely a dryrun for TryTransformTag or second function, otherwise we would count attributes as dropped, that were ignored by TryTransformTag before.

Change requested from open-telemetry#3684 (comment)

* Unittest for LogRecord attribute limits * Remove maxValueLength from LogRecordExtensions.AddIntAttribute. Change requested from #3684 (comment) * Pr commits addressed Co-authored-by: Mikel Blanchard <[email protected]>

* [Logs] Fix buffered log scopes being reused (#3731) * Fix buffered log scopes being reused. * CHANGELOG update. * Test fixes. Co-authored-by: Cijo Thomas <[email protected]> * clarify that Prometheus HttpListner is not for production at this moment (#3737) * [HttpClient] Export spans corresponding to retries (#3732) * Minor update to OTLP readme (#3739) * Nit fixes to prometheus readme * Add minor clarification about OTLP logs * Update workflow (#3741) * Minor improvement to log message (#3742) * [SDK + Jaeger] Support loading environment variables from IConfiguration in Traces & Metrics (#3720) * Support retrieval of environment variables through IConfiguration in SDK. * Update Jaeger to load environment variables through IConfiguration. * Warning fix. * CHANGELOG patch. * Bug fixes. * Warning cleanup. * Code review. * [Zipkin] Support loading envvars from IConfiguration (#3759) * Support loading envvars from IConfiguration in Zipkin exporter. * CHANGELOG patch. * SqlClient Instrumentation to leverage native Activity Status. (#3751) * [Metrics] Update default buckets for Explicit Bucket Histogram from spec (#3722) * [Logs] Fix: Respect AttributeValueLengthLimit when building otlp LogRecord data (#3684) * Respect SdkConfiguration.AttributeValueLengthLimit so otlp data points are not rejected by monitoring tools * Respect maxAttributeCount and use OtlpKeyValueTransformer in AddStringAttribute and in AddIntAttribute * Extend CHANGELOG.md Co-authored-by: Mikel Blanchard <[email protected]> * Bump actions/setup-dotnet from 3.0.1 to 3.0.2 (#3764) Bumps [actions/setup-dotnet](https://github.com/actions/setup-dotnet) from 3.0.1 to 3.0.2. - [Release notes](https://github.com/actions/setup-dotnet/releases) - [Commits](actions/setup-dotnet@v3.0.1...v3.0.2) --- updated-dependencies: - dependency-name: actions/setup-dotnet dependency-type: direct:production update-type: version-update:semver-patch ... Signed-off-by: dependabot[bot] <[email protected]> Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> * [Repo] Attempting to stabilize the API Compatibility CI job (#3766) * Attempting to stablize the API Compatibility CI. * Tweak. * More logging. * Increase build logging level for api compat ci job. * Tweak. * Tweak. * Revert extra logging in ci job. * Switched some logging back to debug. * Adding MinMax to Histograms (#2735) * Logging state during building of TracerProvider (#3746) * [HttpClient] Add unit tests for `RecordException` case (#3761) * Add a separate example project for Logs redaction (#3744) * Update CHANGELOG for 1.4.0-beta.2 release (#3772) * [SDK + Otlp] Support loading envvars from IConfiguration (#3760) * Updated Otlp Trace & Metrics exporters to load envvars from IConfiguration. * Patch CHANGELOGs. * Fix up otlp log exporter for SdkOptions changes. * Revert SdkOptions public api. * Restore tests. * Fix benchmarks. * SdkLimitOptions IConfiguration test. * OtlpExporterOptions IConfiguration test. * MetricReaderOptions IConfiguration test. * Bug fix. * Nit. * [SqlClient] Add support for Filter expression (#3743) * [SDK, Jaeger, Zipkin, & Otlp] Support loading envvars for BatchExportActivityProcessorOptions from IConfiguration (#3776) * Support loading envvars for ExportActivityProcessorOptions & BatchExportActivityProcessorOptions from IConfiguration. * Update src/OpenTelemetry/CHANGELOG.md Co-authored-by: Piotr Kiełkowicz <[email protected]> * Patch CHANGELOG. * Unit test. Co-authored-by: Cijo Thomas <[email protected]> Co-authored-by: Piotr Kiełkowicz <[email protected]> * Remove Env from CI `DOTNET_MULTILEVEL_LOOKUP = 1` (#3779) * Link to .NET docs about different ports (#3704) * Update DS to rc2 (#3781) * ConsoleLogExporter to output full exception (#3784) * [ASP.NET Core] Add back netstandard2.0 and 2.1 targets (#3755) * [HttpClient] Add back netstandard2.0 target (#3787) * Add back netstandard2.0 target * changelog * public api files * Bump System.Text.Json version due to CVE-2021-26701 (#3789) * Auto-generated Semantic Conventions (#2069) * [SDK] Support dependency injection in ResourceBuilder and load envvars from IConfiguration (#3782) * Add Vishwesh as an Approver (#3783) Co-authored-by: Cijo Thomas <[email protected]> * Move more SDK docs to docs folder (#3794) * [Prometheus AspNetCore] Support named options in pipeline extensions (#3780) * Support named options in Prometheus AspNetCore pipeline extensions. * Patch CHANGELOG. Co-authored-by: Cijo Thomas <[email protected]> * [Logs] UnitTest: LogRecord attribute limits (#3758) * Unittest for LogRecord attribute limits * Remove maxValueLength from LogRecordExtensions.AddIntAttribute. Change requested from #3684 (comment) * Pr commits addressed Co-authored-by: Mikel Blanchard <[email protected]> * Add MinMax to console and doc additions (#3795) * Mark private and internal classes as sealed (#3799) Co-authored-by: Cijo Thomas <[email protected]> * Move more docs to docs folder (#3801) * [ASP.NET Core] Update enrich callbacks to use specific type. (#3749) * [Http] Update enrich callbacks for http (#3792) * [SDK] Support dependency injection in the GetDefaultResource API (#3798) * Support dependency injection in the GetDefaultResource API. * CHANGELOG patch. * Test tweaks. Co-authored-by: Cijo Thomas <[email protected]> * Fix circular reference issue building up tracer provider. (#3803) * [SDK] Add some missing nullable annotations (#3796) * Added some missing nullable annotations in SDK. * Handle null span names in SamplingParameters. * Warning cleanup. * Warning cleanup. * Added link to issue in comment. Co-authored-by: Alan West <[email protected]> * Guard.Range now uses invariant culture for error message (#3778) Co-authored-by: Utkarsh Umesan Pillai <[email protected]> * Fix circular reference issue building up meter provider. (#3806) Co-authored-by: Utkarsh Umesan Pillai <[email protected]> * Add missing end of code block backticks (#3807) * More doc tweaks (#3805) * More doc tweaks * remove draft staatus * Update grpc client enrich callbacks (#3804) * Port refactor from main-logs branch. (#3808) Co-authored-by: Cijo Thomas <[email protected]> * Merge fixes. * Merge fixes. * Merge fix. Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: Cijo Thomas <[email protected]> Co-authored-by: Reiley Yang <[email protected]> Co-authored-by: Vishwesh Bankwar <[email protected]> Co-authored-by: Cijo Thomas <[email protected]> Co-authored-by: Yun-Ting Lin <[email protected]> Co-authored-by: Sebastian Schoder Moreno <[email protected]> Co-authored-by: Jonathan Wilhelm <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Co-authored-by: Michael Maxwell <[email protected]> Co-authored-by: ggoel <[email protected]> Co-authored-by: Utkarsh Umesan Pillai <[email protected]> Co-authored-by: Pavel Steinl <[email protected]> Co-authored-by: Piotr Kiełkowicz <[email protected]> Co-authored-by: aristotelos <[email protected]> Co-authored-by: Joao Grassi <[email protected]> Co-authored-by: Alan West <[email protected]> Co-authored-by: benhall_io <[email protected]>

JWilh requested a review from a team September 21, 2022 06:43

JWilh force-pushed the fix/respectAttributeMaxLengthInLoggerExporter branch from b819b7f to d62e8f4 Compare September 21, 2022 06:44

JWilh changed the title ~~Fix: Respect AttributeValueLengthLimit when building otlp LogRecord data~~ [Logs] Fix: Respect AttributeValueLengthLimit when building otlp LogRecord data Sep 21, 2022

JWilh force-pushed the fix/respectAttributeMaxLengthInLoggerExporter branch from d62e8f4 to 0d2fe25 Compare September 28, 2022 09:16

JWilh force-pushed the fix/respectAttributeMaxLengthInLoggerExporter branch 2 times, most recently from f7b52f1 to d0ecd67 Compare October 5, 2022 06:40

Respect SdkConfiguration.AttributeValueLengthLimit so otlp data point…

44725a6

…s are not rejected by monitoring tools

JWilh force-pushed the fix/respectAttributeMaxLengthInLoggerExporter branch from d0ecd67 to 44725a6 Compare October 10, 2022 07:46

alanwest reviewed Oct 10, 2022

View reviewed changes

Jonathan Wilhelm added 2 commits October 11, 2022 08:35

Respect maxAttributeCount and use OtlpKeyValueTransformer in AddStrin…

5a5c758

…gAttribute and in AddIntAttribute

Extend CHANGELOG.md

3caf5a8

alanwest approved these changes Oct 11, 2022

View reviewed changes

JWilh mentioned this pull request Oct 12, 2022

[Logs] UnitTest: LogRecord attribute limits #3758

Merged

3 tasks

utpilla reviewed Oct 13, 2022

View reviewed changes

src/OpenTelemetry.Exporter.OpenTelemetryProtocol/Implementation/LogRecordExtensions.cs Show resolved Hide resolved

Merge branch 'main' into fix/respectAttributeMaxLengthInLoggerExporter

0a503df

CodeBlanch merged commit 0d4c5f5 into open-telemetry:main Oct 13, 2022

CodeBlanch reviewed Oct 13, 2022

View reviewed changes

JWilh pushed a commit to JWilh/opentelemetry-dotnet that referenced this pull request Oct 14, 2022

Remove maxValueLength from LogRecordExtensions.AddIntAttribute.

1d57cef

Change requested from open-telemetry#3684 (comment)

JWilh pushed a commit to JWilh/opentelemetry-dotnet that referenced this pull request Oct 18, 2022

Remove maxValueLength from LogRecordExtensions.AddIntAttribute.

fa21d05

Change requested from open-telemetry#3684 (comment)

JWilh pushed a commit to JWilh/opentelemetry-dotnet that referenced this pull request Oct 19, 2022

Remove maxValueLength from LogRecordExtensions.AddIntAttribute.

ecc330f

Change requested from open-telemetry#3684 (comment)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Logs] Fix: Respect AttributeValueLengthLimit when building otlp LogRecord data #3684

[Logs] Fix: Respect AttributeValueLengthLimit when building otlp LogRecord data #3684

JWilh commented Sep 21, 2022 •

edited

Loading

linux-foundation-easycla bot commented Sep 21, 2022 •

edited

Loading

utpilla commented Sep 21, 2022

JWilh commented Sep 28, 2022

JWilh commented Oct 10, 2022

utpilla commented Oct 10, 2022

codecov bot commented Oct 10, 2022 •

edited

Loading

JWilh commented Oct 10, 2022

alanwest Oct 10, 2022

JWilh Oct 11, 2022

alanwest commented Oct 10, 2022

alanwest commented Oct 10, 2022

JWilh commented Oct 11, 2022 •

edited

Loading

alanwest commented Oct 11, 2022

alanwest left a comment

JWilh commented Oct 12, 2022 •

edited

Loading

JWilh commented Oct 12, 2022

alanwest commented Oct 12, 2022

CodeBlanch commented Oct 13, 2022

CodeBlanch Oct 13, 2022

JWilh Oct 14, 2022 •

edited

Loading

[Logs] Fix: Respect AttributeValueLengthLimit when building otlp LogRecord data #3684

[Logs] Fix: Respect AttributeValueLengthLimit when building otlp LogRecord data #3684

Conversation

JWilh commented Sep 21, 2022 • edited Loading

Changes

linux-foundation-easycla bot commented Sep 21, 2022 • edited Loading

utpilla commented Sep 21, 2022

JWilh commented Sep 28, 2022

JWilh commented Oct 10, 2022

utpilla commented Oct 10, 2022

codecov bot commented Oct 10, 2022 • edited Loading

Codecov Report

JWilh commented Oct 10, 2022

alanwest Oct 10, 2022

Choose a reason for hiding this comment

JWilh Oct 11, 2022

Choose a reason for hiding this comment

alanwest commented Oct 10, 2022

alanwest commented Oct 10, 2022

JWilh commented Oct 11, 2022 • edited Loading

alanwest commented Oct 11, 2022

alanwest left a comment

Choose a reason for hiding this comment

JWilh commented Oct 12, 2022 • edited Loading

JWilh commented Oct 12, 2022

alanwest commented Oct 12, 2022

CodeBlanch commented Oct 13, 2022

CodeBlanch Oct 13, 2022

Choose a reason for hiding this comment

JWilh Oct 14, 2022 • edited Loading

Choose a reason for hiding this comment

JWilh commented Sep 21, 2022 •

edited

Loading

linux-foundation-easycla bot commented Sep 21, 2022 •

edited

Loading

codecov bot commented Oct 10, 2022 •

edited

Loading

JWilh commented Oct 11, 2022 •

edited

Loading

JWilh commented Oct 12, 2022 •

edited

Loading

JWilh Oct 14, 2022 •

edited

Loading