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.

Sign up for GitHub

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

Jump to bottom

Add guid correlation support and change activity processing #169

Merged
merged 14 commits into from
Mar 19, 2020
Merged

Add guid correlation support and change activity processing #169

merged 14 commits into from
Mar 19, 2020

Conversation

AndreyTretyak
Copy link
Contributor

@AndreyTretyak AndreyTretyak commented Mar 17, 2020 •
edited
Loading

  • Move all Activty processing into extensible activity observres
  • Add logic for supporting guid correlation id
  • Change activity API to correspond to future activity changes Improvements to the System.Diagnostics.Activity APIs dotnet/designs#98
  • Switch to using BacgroungService for Omex SF services to reduce the amount of custom code
  • Now all core for using activities inside Abstraction package, add TimedScopes package needed only to log activities

@AndreyTretyak AndreyTretyak linked an issue Mar 18, 2020 that may be closed by this pull request
Investigate and fix possible breaking changes in Extension projects #162
Closed
vlad-ion
vlad-ion requested changes Mar 18, 2020
View reviewed changes
src/Extensions/Abstractions/Activities/Processing/ActivityResultStrings.cs Outdated Show resolved Hide resolved
src/Extensions/Abstractions/Activities/Processing/ActivityResultStrings.cs Outdated Show resolved Hide resolved
src/Extensions/Abstractions/Activities/Processing/ActivityTagKeys.cs Outdated Show resolved Hide resolved
src/Extensions/Abstractions/Activities/TimedScope.cs Outdated Show resolved Hide resolved
src/Extensions/Abstractions/Activities/TimedScopesExtensions.cs Outdated Show resolved Hide resolved
src/Extensions/Abstractions/Activities/TimedScopesExtensions.cs Show resolved Hide resolved
src/Extensions/Logging/Internal/EventSource/ActivityEnhensementObserver.cs Outdated Show resolved Hide resolved
src/Extensions/Logging/ServiceCollectionExtensions.cs Outdated Show resolved Hide resolved
src/Extensions/TimedScopes/ITimedScopeEventSender.cs Show resolved Hide resolved
AndreyTretyak and others added 4 commits March 18, 2020 13:30
@AndreyTretyak @vlad-ion
Update src/Extensions/Abstractions/Activities/Processing/ActivityTagK…
65cb37f 
…eys.cs

Co-Authored-By: Vlad Ion <vlad.ion@gmail.com>
@AndreyTretyak @vlad-ion
Update src/Extensions/Abstractions/Activities/Processing/ActivityResu…
f9b4f40 
…ltStrings.cs

Co-Authored-By: Vlad Ion <vlad.ion@gmail.com>
@AndreyTretyak @vlad-ion
Update src/Extensions/Abstractions/Activities/TimedScope.cs
69be399 
Co-Authored-By: Vlad Ion <vlad.ion@gmail.com>
@AndreyTretyak @vlad-ion
Update src/Extensions/Abstractions/Activities/TimedScopesExtensions.cs
695e173 
Co-Authored-By: Vlad Ion <vlad.ion@gmail.com>
AndreyTretyak and others added 2 commits March 18, 2020 13:40
@AndreyTretyak @vlad-ion
Update src/Extensions/TimedScopes/Internal/TimedScopeEventSender.cs
4c8a0f5 
Co-Authored-By: Vlad Ion <vlad.ion@gmail.com>
@AndreyTretyak @vlad-ion
Update tests/Extensions/Abstractions.UnitTests/Activities/TimedScopeT…
1f380b6 
…ests.cs

Co-Authored-By: Vlad Ion <vlad.ion@gmail.com>
muiriswoulfe
muiriswoulfe reviewed Mar 18, 2020
View reviewed changes
src/Extensions/Abstractions/Activities/ActivityExtensions.cs Outdated
/// <remarks>This property would be transfered to child activity and via web requests</remarks>
[Obsolete(TransactionIdObsoleteMessage, false)]
public static Activity SetObsoleteTransactionId(this Activity activity, uint transactionId) =>
activity.AddBaggage(ObsoleteTransactionId, transactionId.ToString());
Copy link
Member

@muiriswoulfe muiriswoulfe Mar 18, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

ToString [](start = 60, length = 8)

Add CultureInfo.InvariantCulture as a parameter to avoid differences in behaviour on different machines #Closed

Copy link
Contributor Author

@AndreyTretyak AndreyTretyak Mar 19, 2020 •
edited by muiriswoulfe
Loading

Choose a reason for hiding this comment

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

Fixed. Out of curiosity does it actually does anything for int? I'm not aware of different formats for it. #Closed

Copy link
Member

Choose a reason for hiding this comment

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

There are differing number systems around the world, which this could potentially get converted into. See https://en.wikipedia.org/wiki/Numeral_system#/media/File:Numeral_Systems_of_the_World.svg

In reply to: 394708563 [](ancestors = 394708563)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh, I was hoping we have a single standard at least in numbers :) thanks

@muiriswoulfe
Copy link
Member

muiriswoulfe commented Mar 18, 2020 •
edited
Loading 

  	string applicationName,

The parameters here are replicated throughout and then passed directly down the hierarchy of methods as far as I can tell, which is typically considered a code smell. It would be preferable to create a property bag class and pass this around, to avoid all of the replication #Closed

Refers to: src/Extensions/Logging/Internal/EventSource/OmexLogEventSource.cs:15 in 1f380b6. [](commit_id = 1f380b6, deletion_comment = False)

muiriswoulfe
muiriswoulfe reviewed Mar 18, 2020
View reviewed changes
src/Extensions/TimedScopes/ServiceCollectionExtensions.cs
@@ -19,6 +20,9 @@ public static class ServiceCollectionExtensions
public static IServiceCollection AddTimedScopes(this IServiceCollection serviceCollection)
{
Activity.DefaultIdFormat = ActivityIdFormat.W3C;
serviceCollection.AddHostedService<ActivityObserversIntializer>();
serviceCollection.TryAddEnumerable(ServiceDescriptor.Transient<IActivityStopObserver, ActivityStopObserver>());

serviceCollection.TryAddTransient<IActivityProvider, SimpleActivityProvider>();
Copy link
Member

Choose a reason for hiding this comment

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

Is there a reason for the line break? In my mind, it's indicating some difference but I'm not seeing one here

Copy link
Contributor Author

@AndreyTretyak AndreyTretyak Mar 19, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

I've added it to split enumerable registration (hosted is also a special case of enumerable) from Transient, but I don't have a strong opinion and okay removing it if you insisting.

Copy link
Member

Choose a reason for hiding this comment

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

It might be good to add a comment clarifying this for future readers

In reply to: 394724148 [](ancestors = 394724148)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No sure that it worth a comment, it's like splitting different blocks in method, if blank line creates confusion we can just remove it

muiriswoulfe
muiriswoulfe requested changes Mar 18, 2020
View reviewed changes
Copy link
Member

@muiriswoulfe muiriswoulfe left a comment

Choose a reason for hiding this comment

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

🕐

muiriswoulfe
muiriswoulfe approved these changes Mar 19, 2020
View reviewed changes
Copy link
Member

@muiriswoulfe muiriswoulfe left a comment

Choose a reason for hiding this comment

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

:shipit:

@AndreyTretyak AndreyTretyak merged commit 2b0627d into master Mar 19, 2020
@AndreyTretyak AndreyTretyak deleted the feature/correlation-guid-support branch March 19, 2020 11:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@muiriswoulfe muiriswoulfe muiriswoulfe approved these changes

@vlad-ion vlad-ion vlad-ion approved these changes

@armhil armhil Awaiting requested review from armhil

@aszczepanski aszczepanski Awaiting requested review from aszczepanski

@dakanev-msft dakanev-msft Awaiting requested review from dakanev-msft

@davidahs davidahs Awaiting requested review from davidahs

@doguva-msft doguva-msft Awaiting requested review from doguva-msft

@K-Cully K-Cully Awaiting requested review from K-Cully

@neilr81 neilr81 Awaiting requested review from neilr81

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Investigate and fix possible breaking changes in Extension projects
3 participants
@AndreyTretyak @muiriswoulfe @vlad-ion