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: extend metrics with metadata #231

Merged
merged 1 commit into from
Jul 9, 2024
Merged

Conversation

sighphyre
Copy link
Member

@sighphyre sighphyre commented Jul 9, 2024

Needs #229, otherwise it'll be noisy

This adds metadata to the metrics and registration calls. This has a few sharp edges to it:

  • Insofar as I can tell, there's no reliable way to get the version of the CLR or framework when running in C#. Everything has some details where it won't work on something. I've chosen to go with some degenerate string splitting + a compiler directive for older versions, which should give us enough information to know what's happening in the wild and always return "something"
  • The design of the way async tasks work in this SDK needs to be rewritten to reduce the coupling between async stuff and business logic stuff. Right now there's no clean way to separate these two, which means there's no good way to extend the metrics/registration in a way that isn't imminently breakable from the consumer perspective or testable in any rational way. Because of this, I've chosen to make these properties lazy getters on the Metrics/Registration objects themselves because it's hard to accidentally break and it gives us some testing

The task system needs some love but I don't think it's responsible to do that before Yggdrasil gets merged

@sighphyre sighphyre changed the base branch from main to chore/gitignore-for-vs-code-backup July 9, 2024 11:24
@sighphyre sighphyre changed the base branch from chore/gitignore-for-vs-code-backup to main July 9, 2024 11:25
@sighphyre sighphyre force-pushed the feat/metrics-extended branch 4 times, most recently from 8eb7d1b to 31f3cbc Compare July 9, 2024 11:36
Copy link
Member

@nunogois nunogois left a comment

Choose a reason for hiding this comment

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

Shame there's no better way of getting this information, but this looks good 👍

@sighphyre
Copy link
Member Author

Shame there's no better way of getting this information, but this looks good 👍

+1! I think we can make it more complex to get richer information but it doesn't feel worth it right now

@sighphyre sighphyre merged commit 7c639dd into main Jul 9, 2024
2 checks passed
@sighphyre sighphyre deleted the feat/metrics-extended branch July 9, 2024 12:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

2 participants