Update temporal integration generate_metadata.py script #20469
Conversation
| metric_name = match.group(3) | ||
|
|
||
| # Extract type from function name (everything between New and Def) | ||
| type_name = func_name[3:-3] # Remove "New" prefix and "Def" suffix |
There was a problem hiding this comment.
Would it be possible to tweak the regex in such a way that we don't need to do this extraction later?
For instance have something like ...New(\w*)Def.
temporal-github-worker-1
bot
added
agent/requested-changes
and removed
agent/review-requested
labels
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
phuongdnguyen
force-pushed
the
update-temporal-generate-metadata-script
branch
from
9314a78 to
dd8fb92
Compare
| "and is not present in the current metadata.csv file" | ||
| ) | ||
| continue | ||
|
|
||
| try: | ||
| temporal_type = temporal_metric_types[temporal_name] | ||
| except KeyError: |
There was a problem hiding this comment.
How exceptional is this? How often do we except it to happen?
There was a problem hiding this comment.
Agree, use a cleaner lookup mechanism here
| tuple: (metadata list with any existing metrics added, boolean indicating if metric exists) | ||
| """ | ||
| pattern = re.compile(rf"^temporal\.server\.{re.escape(name)}(?:\.[a-z]+)*$") | ||
| exist = False |
There was a problem hiding this comment.
You don't need this variable since it's the same as bool(result). If result is not empty, then exist is always True, and if result is empty, then exist is False.
| # If metrics does not exist in this Temporal version, try to search metric in | ||
| # the current metadata file and preserve it if it's already exist | ||
| existing_dd_metric, existed_dd_metric = check_existing_metric( | ||
| dd_metric, previous_metadata, added_dd_metrics | ||
| ) | ||
| if existed_dd_metric: | ||
| metadata.extend(existing_dd_metric) | ||
| else: | ||
| print( | ||
| f"WARNING: skipping metric `{temporal_name}/{dd_metric}` because it's " | ||
| "not present in both temporal metric definitions and the current " | ||
| "metatada.csv file" | ||
| ) | ||
| continue |
There was a problem hiding this comment.
Can we get rid of the duplication between this and the block above it?
There was a problem hiding this comment.
I'm not sure we should remove it. Those have different meaning: not processing and log out its existent vs not processing because it's not exist
phuongdnguyen
force-pushed
the
update-temporal-generate-metadata-script
branch
from
5d805cd to
ee98200
Compare
phuongdnguyen
force-pushed
the
update-temporal-generate-metadata-script
branch
from
ee98200 to
1634997
Compare
What does this PR do?
Update the metadata generation script to:
metadata.csvfile.Motivation
This PR is split from #20142, the purpose is for us to able to support metrics in recent version of temporal.
Related support request https://help.datadoghq.com/hc/en-us/requests/2152576
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