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

[hotfix-sdk] Fix reflection breaks introduced by new Batch ctor #5994

Closed

Conversation

CodeBlanch
Copy link
Member

@CodeBlanch CodeBlanch commented Nov 20, 2024

Changes

  • Switch the public Batch(T item) ctor introduced in 1.10.0-rc.1 back to internal
  • Introduce static Batch<T> Create(T item) instead

Details

We promoted the internal Batch<T> ctor which accepts on single item to public so that components like GenevaExporter wouldn't need to call it with reflection. The problem is the shipped versions which call it with reflection now break if the 1.10.0 SDK is present. To fix this we are putting the ctor back to internal and introducing a static method instead. The plan is to release 1.10.1 with this fix. Then obsolete/unlist GenevaExporter 1.10.0 and replace it with a 1.10.1 version which will call the new static method. We will probably also obsolete/unlist 1.10.0 SDK but need to discuss first.
 

Merge requirement checklist

  • CONTRIBUTING guidelines followed (license requirements, nullable enabled, static analysis, etc.)
  • Unit tests added/updated
  • Appropriate CHANGELOG.md files updated for non-trivial changes
  • Changes in public API reviewed (if applicable)

@CodeBlanch CodeBlanch requested a review from a team as a code owner November 20, 2024 01:12
@github-actions github-actions bot added the pkg:OpenTelemetry Issues related to OpenTelemetry NuGet package label Nov 20, 2024
/// </summary>
/// <param name="item">Item to store in the batch.</param>
/// <returns><see cref="Batch{T}"/> containing the supplied item.</returns>
[MethodImpl(MethodImplOptions.AggressiveInlining)]
Copy link
Member Author

@Kielek
Copy link
Contributor

Kielek commented Nov 20, 2024

I do not think that it is good idea. It is a risk on the consumer side if they are relaying on non-public stuff.
I doubt that it is big adoption of 1.10.0 in the wild, but introducing binary breaking changes on public contract is something we should avoid.

EDIT: if it affects only GenevaExporter, lets make some messages internally in MS that it requires bumping both packages together.

@reyang
Copy link
Member

reyang commented Nov 20, 2024

...but introducing binary breaking changes on public contract is something we should avoid.

+1

@alanwest
Copy link
Member

I do not think that it is good idea.

I agree with @Kielek. The design of our public APIs should not be influenced by libraries that use our packages in unorthodox ways (e.g.,, accessing non-public constructors). This pattern of a static Create method doesn't feel like a pattern we use very widely in the SDK.

Originally, I supported making the constructor public because I think it is reasonable to give extension authors access to things we use in the extensions we ship with the SDK like the simple export processor. That is, while the Geneva exporter motivated the need, it is not what motivated my decision.

Copy link
Member

@alanwest alanwest left a comment

Choose a reason for hiding this comment

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

Blocking due to concerns raised by @Kielek and myself.

@reyang
Copy link
Member

reyang commented Nov 20, 2024

...but introducing binary breaking changes on public contract is something we should avoid.

+1

should -> must

@CodeBlanch CodeBlanch closed this Nov 21, 2024
@CodeBlanch CodeBlanch deleted the sdk-revert-public-batch-ctor branch November 21, 2024 18:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pkg:OpenTelemetry Issues related to OpenTelemetry NuGet package
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants