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

(WIP) Separate property bag of TelemetryContext and ISupportProperties #814

Closed
wants to merge 4 commits into from

Conversation

cijothomas
Copy link
Contributor

To get early feedback only. This may be treated as breaking change and require Major version update.

Summary:
All the current Telemetry types defined in base sdk (eg: RequestTelemetry, DependencyTelemetry) implements ISupportProperties, which gives them a Properties bag (string,string). ITelemetry also has TelemetryContext which also has a Property bag. Currently these two property bags are internally referring to the same collection. ( so modifying one affects the other.)

This PR changes the implementation so that the Properties from ISupportProperties and TelemetryContext are now independent collection in each of the concrete ITelemetry implementations. Though not a breaking change, this has the effect that, if any customer depended on these two property collection being the same, they'll have undesired consequences.

Also, this does not change serialization logic for now. Both the property bags are serialized into the "properties" of the telemetry. But this may eventually change so as to 'promote' the properties from TelemetryContext to be outside the item itself.

For significant contributions please make sure you have completed the following items:

  • Design discussion issue #
  • Changes in public surface reviewed
  • CHANGELOG.md updated with one line description of the fix, and a link to the original issue.
  • The PR will trigger build, unit test, and functional tests automatically. If your PR was submitted from fork - mention one of committers to initiate the build for you.

…s, and ISupportProperties.Properties, and corresponding unit test changes.
…ods to apply custom properties to the item directly
@lmolkova
Copy link
Member

  • I believe we have mixed usage of different properties in web SDK and I assume everywhere else. Is there any plan to ensure all our SDKs use the right thing?

  • What is the motivation behind this change?

  • It is actually confusing already and luckily they are the same and it does not matter, but if they become different things, I'd propose them to have different names

@pharring
Copy link
Member

I have the same question as Liudmila. Why?
Doesn't separating them makes it more confusing for consumers (and the rest of the SDK)?
i.e. Which one should I use? What if you write the same property with different values to each collection? Which one wins?

@SergeyKanzhelev
Copy link
Contributor

@cijothomas it would be great if you'll move discussion to the issue. PR are not as well searcheable.

@cijothomas
Copy link
Contributor Author

All - Sorry for not describing more context. Will open an issue with more details today :)

@cijothomas
Copy link
Contributor Author

@lmolkova @pharring @SergeyKanzhelev Lets use #816 for higher level discussion, and the PR for code-comments.

@cijothomas
Copy link
Contributor Author

Abandoning in favor of #820 as it lets us do this without breaking customers.

@cijothomas cijothomas closed this May 30, 2018
@TimothyMothra TimothyMothra deleted the cithomas/schemasplit branch September 13, 2018 16:52
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