-
Notifications
You must be signed in to change notification settings - Fork 1k
Introduce telemetry context and proper use of ILogger #3213
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
Conversation
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #3213 +/- ##
==========================================
+ Coverage 57.55% 57.64% +0.08%
==========================================
Files 356 361 +5
Lines 78784 79152 +368
Branches 13870 13814 -56
==========================================
+ Hits 45347 45628 +281
- Misses 29193 29301 +108
+ Partials 4244 4223 -21 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
...ecurity.Certificates/System.Security.Cryptography.X509Certificates/X509SignatureGenerator.cs
Show resolved
Hide resolved
Libraries/Opc.Ua.Server/Subscription/MonitoredItem/MonitoredItem.cs
Outdated
Show resolved
Hide resolved
romanett
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed first 160 files
romanett
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
reviewed first 273 files (only test projects missing). Can approve if comments are answered
| @@ -533,29 +309,135 @@ public object Clone() | |||
| /// <inheritdoc/> | |||
| public new object MemberwiseClone() | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As I understand this new MemberwiseClone() will cause the new clone to share the underlying the new ConcurrentDictionary m_encodeableTypes = encodeableFactory.m_encodeableTypes;. Quite a difference in behaviour with the previous implementation. Doesn't the copy-on-write mechanism cause a cloned instance to create a copy only if the cloned instance changes, otherwise a clone shares the changes with the object it has been cloned from if changes have been done on parent ? Maybe this needs a special mention in the document (if assumption is true and not already there). Otherwise great stuff.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, it is not immutable. The next change would be to make all of it use an immutable or frozen dictionary and share the pointer at time of clone.
mrsuciu
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great stuff.
Proposed changes
Information in the Observability.md markdown inside this PR.
This is part 1, further changes are made once .net standard samples are updated to use this as preview nuget.
Related Issues
Types of changes
What types of changes does your code introduce?
Put an
xin the boxes that apply. You can also fill these out after creating the PR.Checklist
Put an
xin the boxes that apply. You can also fill these out after creating the PR. If you're unsure about any of them, don't hesitate to ask. We're here to help! This is simply a reminder of what we are going to look for before merging your code.Further comments
If this is a relatively large or complex change, kick off the discussion by explaining why you chose the solution you did and what alternatives you considered, etc...