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

feat: Ambiguous method definition on LogMetric #424

Merged
merged 17 commits into from
Jul 14, 2022

Conversation

vincenttermaat
Copy link
Contributor

Ambiguous method definition on LogMetric
Closes #280

  • Update Arcus.Testing.Logging 0.3.0 > 0.4.0
  • Rename LogMetric to LogCustomMetric
  • Mark LogMetric as obsolete
  • Rename LogEvent to LogCustomEvent
  • Mark LogEvent as obsolete

@netlify
Copy link

netlify bot commented Jun 29, 2022

Deploy Preview for arcus-observability ready!

Name Link
🔨 Latest commit 70566aa
🔍 Latest deploy log https://app.netlify.com/sites/arcus-observability/deploys/62c81b47eaeceb00088ad332
😎 Deploy Preview https://deploy-preview-424--arcus-observability.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

Copy link
Member

@stijnmoreels stijnmoreels left a comment

Choose a reason for hiding this comment

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

We should also reflect these changes in the feature documentation so people are guided towards these new extensions.
Thx again for your work! 💯 Really help us with all these many tasks across repo's (that we often forget 😄 ).

vincenttermaat and others added 9 commits July 4, 2022 11:33
…Extensions.cs

Co-authored-by: Stijn Moreels <9039753+stijnmoreels@users.noreply.github.com>
…cExtensions.cs

Co-authored-by: Stijn Moreels <9039753+stijnmoreels@users.noreply.github.com>
…cExtensions.cs

Co-authored-by: Stijn Moreels <9039753+stijnmoreels@users.noreply.github.com>
Revert removal unit-tests
Add unit-tests
@vincenttermaat vincenttermaat removed their assignment Jul 4, 2022
@vincenttermaat
Copy link
Contributor Author

We should also reflect these changes in the feature documentation so people are guided towards these new extensions. Thx again for your work! 💯 Really help us with all these many tasks across repo's (that we often forget 😄 ).

starting on this

@vincenttermaat
Copy link
Contributor Author

We should also reflect these changes in the feature documentation so people are guided towards these new extensions. Thx again for your work! 💯 Really help us with all these many tasks across repo's (that we often forget 😄 ).

starting on this

3dde0c3

Copy link
Member

@stijnmoreels stijnmoreels left a comment

Choose a reason for hiding this comment

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

This is fine for me. Thx for the update! Looks great. 😄

@stijnmoreels
Copy link
Member

Woops, we're having some conflicts in the feature documentation, @vtermaat .
Probably bc we have changed the order of the files/folders. Normally, Git should be smart enough, so if you would update your master locally, and merge it back into this branch, it should resolve (easily) (hopefully, 😅 ).

@fgheysels
Copy link
Member

What about the rename of LogEvent to LogCustomEvent. Should we keep that or revert that @stijnmoreels ?

@stijnmoreels
Copy link
Member

What about the rename of LogEvent to LogCustomEvent. Should we keep that or revert that @stijnmoreels ?

It was added here as part of the LogMetric > LogCustomMetric change to streamline our extensions. It's fine for me as it supports consistency and will maybe be easier to find as it's also called 'custom events' in Application Insights, right?

@fgheysels
Copy link
Member

What about the rename of LogEvent to LogCustomEvent. Should we keep that or revert that @stijnmoreels ?

It was added here as part of the LogMetric > LogCustomMetric change to streamline our extensions. It's fine for me as it supports consistency and will maybe be easier to find as it's also called 'custom events' in Application Insights, right?

I actually thought that it was just called an 'event' in AppInsights. However, I looked around and indeed, they're saved to a table called customEvents, and in the documentation they also (sometimes) refer to it as custom events.
I had some doubts about this (see also what I mentionned in the issue), but OK.

I can agree with this. (Sorry for being a bit 'picky' sometimes when we're discussing things about naming stuff).

@fgheysels fgheysels merged commit 268efd2 into arcus-azure:master Jul 14, 2022
@stijnmoreels
Copy link
Member

I can agree with this. (Sorry for being a bit 'picky' sometimes when we're discussing things about naming stuff).

Hihi, 😄 , no need to apologize, I'm glad I'm not the only one to be picky on this. 😉 Glad that we find each other in this.

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.

Ambiguous method definition on LogMetric
3 participants