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

[Instrumentation.MySqlData] Nullable #1094

Merged
merged 1 commit into from
Mar 20, 2023

Conversation

Kielek
Copy link
Contributor

@Kielek Kielek commented Mar 20, 2023

Towards #894.

Changes

[Instrumentation.MySqlData] Nullable.
Part of shared files also updated.

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

@Kielek Kielek marked this pull request as ready for review March 20, 2023 12:42
@Kielek Kielek requested a review from a team March 20, 2023 12:42
@Kielek
Copy link
Contributor Author

Kielek commented Mar 20, 2023

@moonheart, could you please check?

@moonheart
Copy link
Member

This LGTM 👍

@utpilla utpilla added the comp:instrumentation.mysqldata Things related to OpenTelemetry.Instrumentation.MySqlData label Mar 20, 2023
@@ -41,7 +41,7 @@ public static class TracerProviderBuilderExtensions
/// <returns>The instance of <see cref="TracerProviderBuilder"/> to chain the calls.</returns>
public static TracerProviderBuilder AddMySqlDataInstrumentation(
this TracerProviderBuilder builder,
Action<MySqlDataInstrumentationOptions> configure)
Action<MySqlDataInstrumentationOptions>? configure)
Copy link
Contributor

Choose a reason for hiding this comment

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

We should probably not put ? to stay consistent with other instrumentation libraries.

Look at this for reference: https://github.com/open-telemetry/opentelemetry-dotnet/blob/main/src/OpenTelemetry.Instrumentation.AspNetCore/TracerProviderBuilderExtensions.cs#L59-L71

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have discussion with @alanwest some time ago. The consensus is to make it nullable non-optional.

See #913

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah okay. Thanks for sharing that.

@Kielek Kielek merged commit 9b8a774 into open-telemetry:main Mar 20, 2023
@Kielek Kielek deleted the mysql-nullable branch March 20, 2023 17:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
comp:instrumentation.mysqldata Things related to OpenTelemetry.Instrumentation.MySqlData
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants