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

deep copy empty attributes #714

Merged
merged 4 commits into from
May 27, 2020

Conversation

AndrewAXue
Copy link
Contributor

@AndrewAXue AndrewAXue commented May 20, 2020

Addresses #713. Previously it was possible for a user (acting against the api) to mutate a default variable. Deepcopying solves this issue.

@AndrewAXue AndrewAXue requested a review from a team May 20, 2020 19:40
Copy link
Member

@toumorokoshi toumorokoshi left a comment

Choose a reason for hiding this comment

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

I agree that this approach will fix the issue.

One fix and I think it's good to merge: can you create utility methods rather than have parameterized creation? It's pretty easy to pass the wrong variable and create the wrong object.

I'd suggest public utility methods around empty_{attributes,events,links}.

Copy link
Member

@toumorokoshi toumorokoshi left a comment

Choose a reason for hiding this comment

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

thanks! I'm not sure that these attributes being protected is the right approach, but until we find a need to create such empty sets outside of the constructor, I think we're ok.

Copy link
Contributor

@codeboten codeboten left a comment

Choose a reason for hiding this comment

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

Can you add a note to the sdk changelog, i'll approve and merge afterwards

Copy link
Contributor

@codeboten codeboten left a comment

Choose a reason for hiding this comment

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

Thanks! 👍

@codeboten codeboten merged commit 8a946b5 into open-telemetry:master May 27, 2020
srikanthccv pushed a commit to srikanthccv/opentelemetry-python that referenced this pull request Nov 1, 2020
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.

4 participants