-
Notifications
You must be signed in to change notification settings - Fork 171
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
3185 unify pipeline tag #3191
3185 unify pipeline tag #3191
Conversation
731b972
to
5abfd40
Compare
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.
LGTM.
@rickstaa should we just merge it or should we notify anyone that the label is changed?
c1e342a
to
e94b188
Compare
This commit ensures that the new metrics functions also use the `normalizePipelineTag` method to ensure consistent pipeline naming in the metrics.
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.
@pwilczynskiclearcode Great catch! I've implemented your suggested change to the new AI remote worker metrics functions. One additional point to consider is whether we want to ensure consistent behavior in the ai_process.go file. Currently, we're inserting a capability string instead of a pipeline string, as we are in census.go.
I'm okay with handling this discrepancy in the census package for now, but if we think it's better to also address this in the core codebase, we can include a change in a subsequent pull request. @leszko, @pwilczynskiclearcode — let me know what you think!
@pwilczynskiclearcode, @leszko we can also change |
Added one commit 421aa2c I think I like the suggestion from @rickstaa to have this conversion outside the census code. We should use pipeline naming consistently through the code. @pwilczynskiclearcode If you like my change, feel free to merge (after all CI builds pass). |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## ai-video #3191 +/- ##
====================================================
- Coverage 50.82671% 36.07820% -14.74851%
====================================================
Files 100 124 +24
Lines 23769 34525 +10756
====================================================
+ Hits 12081 12456 +375
- Misses 11018 21381 +10363
- Partials 670 688 +18
... and 25 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
|
What does this pull request do? Explain your changes.
Unifies the "pipeline" prometheus metric tag produced by
AIRequestError
Fixes #3185
Checklist:
make
runs successfully./test.sh
pass