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

Split common package into value and scope #445

Closed
wants to merge 1 commit into from

Conversation

bogdandrutu
Copy link
Member

This is a no-op for generated code, but helps within the collector pdata where we want to replace the generated code for the value (remove the entire value file essentially).

Signed-off-by: Bogdan Drutu bogdandrutu@gmail.com

This is a no-op for generated code, but helps within the collector pdata where we want to replace the generated code for the value (remove the entire value file essentially).

Signed-off-by: Bogdan Drutu <bogdandrutu@gmail.com>
Copy link
Member

@tigrannajaryan tigrannajaryan left a comment

Choose a reason for hiding this comment

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

Please add a link to Collector pdata issue that requires this change.

opentelemetry/proto/common/v1/scope.proto Show resolved Hide resolved
@yurishkuro
Copy link
Member

This is a no-op for generated code

Are you sure? May have different effect in different languages, e.g. produce scope.py/value.py in Python that must be imported separately (not saying it would be a reason not to do this).

@bogdandrutu
Copy link
Member Author

Are you sure? May have different effect in different languages, e.g. produce scope.py/value.py in Python that must be imported separately (not saying it would be a reason not to do this).

You are right, did not think about this. Probably not worth it then. I will do a hack on the collector.

@bogdandrutu
Copy link
Member Author

@jsuereth @tigrannajaryan since you want to declare proto 1.0, I want to document if this change is considered a breaking change or not.

@jsuereth
Copy link
Contributor

I would like to NOT consider this a breaking change, but I'd also like to avoid changes like this post 1.0.

The cost of cosmetic changes like this can often outweigh the benefits, even in the long run. However, at this time, I think this may be the last point in time where I'd be supportive of a change like this.

@tigrannajaryan
Copy link
Member

Are you sure? May have different effect in different languages, e.g. produce scope.py/value.py in Python that must be imported separately (not saying it would be a reason not to do this).

You are right, did not think about this. Probably not worth it then. I will do a hack on the collector.

Closing this since @bogdandrutu no longer needs it.

@jsuereth @tigrannajaryan since you want to declare proto 1.0, I want to document if this change is considered a breaking change or not.

I think this is considered a breaking change in the proposed PR: #432

We can further discuss on that PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants