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

Add notes for System.Diagnostics.Activity #117

Merged
merged 1 commit into from
Apr 17, 2020

Conversation

terrajobst
Copy link
Member

No description provided.

@terrajobst terrajobst requested a review from a team as a code owner April 14, 2020 18:54
@terrajobst terrajobst force-pushed the system-diagnosics-activity branch from 51c2e39 to b452b71 Compare April 14, 2020 18:58
## Notes

* `ActivityKind`
* Looks like `ActivityKind.Internal` is supposed to be the default which
Copy link
Member

Choose a reason for hiding this comment

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

We decided to set Internal=0 and shift the rest.

* Looks like `ActivityKind.Internal` is supposed to be the default which
[Python defines][python_activity] as `0`. We should do the same.
* Should we put these API in a namespace of `System.Diagnostics`? This
namespace already contains many technologies.
Copy link
Member

Choose a reason for hiding this comment

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

We decided to keep the APIs in System.Diagnostics because the Activity class is in this namespace.

`right`
* We should name the parameter of the strongly typed `Equals` as `value`
* `ActivityEvent`
* Can we make this a `struct`?
Copy link
Member

Choose a reason for hiding this comment

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

We decided to change this to struct instead of class.

Dispose(bool disposing)` and `Dispose()` should call `Dispose(true)`.
* `SetCustomProperty` and `GetCustomProperty` have no way to differentiate
between a property is set to `null` vs. a property wasn't set at all. Is
this OK?
Copy link
Member

Choose a reason for hiding this comment

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

We decided it is OK for this scenario.

`right`
* We should name the parameter of the strongly typed `Equals` as `value`
* Should `IDictionary<string, string>` be a `IReadOnlyDictionary<string, string>`?
* Should the `attributes` parameter be optional and defaulted to `null`?
Copy link
Member

Choose a reason for hiding this comment

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

Yes, we decided to go with the optional parameters.

* We should name the parameters of `operator==` and `operator!=` as `left` and
`right`
* We should name the parameter of the strongly typed `Equals` as `value`
* Should `IDictionary<string, string>` be a `IReadOnlyDictionary<string, string>`?
Copy link
Member

@tarekgh tarekgh Apr 14, 2020

Choose a reason for hiding this comment

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

I have reviewed the OpenTelemetry specs and it requires the attributes be defined as the tags and preserve the order too. So, I think either we need to make this as IEnumerable or we'll consider the suggestion mentioned in on StartActivity to have something like ActivtyTagsCollection

@tarekgh
Copy link
Member

tarekgh commented Apr 14, 2020

@terrajobst do we consider this conditionally approved? of course, we'll report back by the email when applying the feedback comments and have a final design for the listener API.

@terrajobst
Copy link
Member Author

@terrajobst do we consider this conditionally approved?

Yes, which is what the notes say at the top ("Approved with feedback"). I should have marked the issue as approved too, but didn't. Will do no!

@terrajobst terrajobst merged commit 009bdb6 into dotnet:master Apr 17, 2020
@terrajobst terrajobst deleted the system-diagnosics-activity branch April 18, 2020 03:02
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.

2 participants