-
Notifications
You must be signed in to change notification settings - Fork 4
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
[SDTEST-415] Add test_session metric #207
Conversation
@@ -18,6 +18,23 @@ module Telemetry | |||
Ext::AppTypes::TYPE_TEST_SESSION => Ext::Telemetry::EventType::SESSION | |||
}.freeze | |||
|
|||
PROVIDER_TAG_TO_TELEMETRY_PROVIDER_TAG = { |
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.
This is incredibly ugly: for some reason we use different values for some providers when submitting to citestcycle and telemetry. For example Microsoft Azure is "azurepipelines" in citestcylce but "azp" in telemetry
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.
Hot take: I think we shouldn't do this and I think we should fix the other tracers to use the same tags for telemetry and citestcycle.
Unless I'm mistaken, the backend doesn't have an enum validating these values, so worst case, we'd have some extra values for some time.
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.
I like your proposal, but I also would like to move fast and leave this epic behind. Given that the discussion about naming can go on for months from now, let's merge this one as is and then I'll fix it in one of the subsequent PRs
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #207 +/- ##
=======================================
Coverage 98.84% 98.85%
=======================================
Files 251 251
Lines 11526 11576 +50
Branches 516 516
=======================================
+ Hits 11393 11443 +50
Misses 133 133 ☔ View full report in Codecov by Sentry. |
@@ -18,6 +18,23 @@ module Telemetry | |||
Ext::AppTypes::TYPE_TEST_SESSION => Ext::Telemetry::EventType::SESSION | |||
}.freeze | |||
|
|||
PROVIDER_TAG_TO_TELEMETRY_PROVIDER_TAG = { |
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.
Hot take: I think we shouldn't do this and I think we should fix the other tracers to use the same tags for telemetry and citestcycle.
Unless I'm mistaken, the backend doesn't have an enum validating these values, so worst case, we'd have some extra values for some time.
What does this PR do?
Adds test_session metric with provider tag to track which CI providers are used
How to test the change?
Unit tests