Update temporal check to temporal v1.28.0#20627
Update temporal check to temporal v1.28.0#20627phuongdnguyen wants to merge 32 commits intoDataDog:masterfrom
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files🚀 New features to boost your workflow:
|
|
Hi @iliakur , appreciate your review 🙇♂️ |
| aggregator.assert_metric_has_tag(metric, tag) | ||
|
|
||
| aggregator.assert_metrics_using_metadata(get_metadata_metrics()) | ||
| aggregator.assert_all_metrics_covered() |
There was a problem hiding this comment.
Remove this assertion because we must keep the integration working against several upstream versions - where some metrics appear or disappear between releases and the test already covered a subset of core metrics, wdyt @iliakur ?
There was a problem hiding this comment.
I'm -1 to do this in a unit test where we should have full control over the payloads we send.
As I see it we have 2 options:
- test each version with a separate test and a dedicated payload
- have one big payload "to rule them all" that contains data for all supported versions
I prefer option 2 and can help set that up.
There was a problem hiding this comment.
Cool @iliakur , please help with the testing setup, thanks thanks 🙇♂️
There was a problem hiding this comment.
Hi @iliakur , any ETA for the testing set up ? 🙇
There was a problem hiding this comment.
sorry, next week at this point
There was a problem hiding this comment.
@phuongdnguyen I haven't forgotten, just behind on tasks right now. I'll reach out as soon as I find time.
There was a problem hiding this comment.
No prob @iliakur , I'm also can only get back to this after this Thurday also
There was a problem hiding this comment.
Hi @iliakur , I've merged the scripts as you suggested
There was a problem hiding this comment.
Hi @iliakur , can we can get on this ? :)
iliakur
left a comment
iliakur
left a comment
There was a problem hiding this comment.
thanks again for your hard work 💪
| aggregator.assert_metric_has_tag(metric, tag) | ||
|
|
||
| aggregator.assert_metrics_using_metadata(get_metadata_metrics()) | ||
| aggregator.assert_all_metrics_covered() |
There was a problem hiding this comment.
I'm -1 to do this in a unit test where we should have full control over the payloads we send.
As I see it we have 2 options:
- test each version with a separate test and a dedicated payload
- have one big payload "to rule them all" that contains data for all supported versions
I prefer option 2 and can help set that up.
There was a problem hiding this comment.
Strongly in favor of merging the two scripts and having 2 separate commands.
Or should we even just have this be part of the same command? in my understanding the two actions always come together, right?
There was a problem hiding this comment.
I think we can merge it into 1 script with 2 commands.
There is a case when the 2 operations does not comes together: native dynamic metrics is added in METRICS_MAP manually, then developer want to generate the metadata from it only.
There was a problem hiding this comment.
would it make sense to have just one command that does both actions and then add a flag that disables the first action?
|
Hi @iliakur, any chance we can push this through? 🙏 Would love to have metrics from the newer Temporal versions. Thanks! |
4 participants
What does this PR do?
Motivation
Support metrics in newer temporal version as OOTB
Split from #20142
First part: #20469
Review checklist (to be filled by reviewers)
qa/skip-qalabel if the PR doesn't need to be tested during QA.backport/<branch-name>label to the PR and it will automatically open a backport PR once this one is merged