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

[Exporter.Geneva] Update OTel SDK prerelease version #1210

Merged
merged 13 commits into from
Jun 2, 2023

Conversation

utpilla
Copy link
Contributor

@utpilla utpilla commented May 26, 2023

Changes

  • Update OTel SDK prerelease version to 1.5.0-rc.1
  • Suppress warnings about using Obsolete code for LogRecord.State and LogRecord.StateValues
  • Update AddGenevaTraceExporter and AddGenevaMetricExporter methods
  • Appropriate CHANGELOG.md updated for non-trivial changes

@utpilla utpilla changed the title Update OTel SDK prerelease version [Exporter.Geneva] Update OTel SDK prerelease version May 26, 2023
@@ -138,6 +138,8 @@ internal bool IsUsingUnixDomainSocket

internal int SerializeLogRecord(LogRecord logRecord)
{
// `LogRecord.State` and `LogRecord.StateValues` were marked Obsolete in https://github.com/open-telemetry/opentelemetry-dotnet/pull/4334
#pragma warning disable 0618
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@cijothomas @CodeBlanch Okay with this change?

Copy link
Member

Choose a reason for hiding this comment

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

Yes. Doing this suppress-warning, will be yet another validation that the main sdk has done everything in back-compat way!

@utpilla utpilla added the comp:exporter.geneva Things related to OpenTelemetry.Exporter.Geneva label May 26, 2023
AddGenevaTraceExporter(builder, sp.GetOptions<GenevaExporterOptions>(), configure);
});
}
var exporterOptions = sp.GetOptions<GenevaExporterOptions>();
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@CodeBlanch I have updated the extension methods for both GenevaTraceExporter and GenevaMetricExporter to use the newer DI APIs. Could you please review these changes?

Copy link
Member

Choose a reason for hiding this comment

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

This looks good.

There are a couple things missing, but up to you if you want to shore it up 😄 I'll just drop details here you can tackle now or later if you want.

  1. Named options support is missing. Probably doesn't seem important, but I think because all signals share an options class it actually is very useful for GenevaExporter (& OtlpExporter).
  2. Options API integration won't work totally as expected.

I think 1 is pretty self-explanatory so I won't go into it but 2 maybe not as clear; here is an example:

appBuilder.Services.PostConfigure<GenevaExporterOptions>(o =>
{
    o.ConnectionString = "[connection_string_2]";
});

appBuilder.Services.AddOpenTelemetry().WithTracing(builder =>
   builder.AddGenevaTraceExporter(o => {
      o.ConnectionString = "[connection_string_1]";
   }));

In that snippet the user probably expected the "PostConfigure" delegate to run last. But the way the code is written, the inline one will win.

The reason for that is on line 40 below where we do configure?.Invoke(options); that is executing it inline and not participating correctly in the Options API world.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for sharing this insight! I'll address these in a different PR.

@github-actions github-actions bot requested a review from CodeBlanch June 2, 2023 21:25
@utpilla utpilla merged commit 0b90344 into open-telemetry:main Jun 2, 2023
@utpilla utpilla deleted the utpilla/Update-OTel-Sdk-version branch August 25, 2023 23:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
comp:exporter.geneva Things related to OpenTelemetry.Exporter.Geneva
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants