Replies: 6 comments 5 replies
-
|
@cbuffett |
Beta Was this translation helpful? Give feedback.
-
|
@cbuffett |
Beta Was this translation helpful? Give feedback.
-
|
We can have the key:value/key=value based tags in DAGs. While emitting the metrics with dag_id tag key, then we can enrich it with additional dag tags. We can skip the dag tags that don't follow the key-value format. |
Beta Was this translation helpful? Give feedback.
-
|
The idea is interesting, and I'm not against it at all if it'll work, but it's maybe not trivial. What would be the expected order of priority if the user adds a tag with an existing key, for example? |
Beta Was this translation helpful? Give feedback.
-
|
I don't expect user to use the same key. If they pass the same key, we can clearly call out, the behavior is undefined. |
Beta Was this translation helpful? Give feedback.
-
|
We can't assume the user won't break something, we have to plan for it. We definitely don't want Airflow to throw a runtime exception and crash over a metric tag, so we'd need some sort of established way to handle it. Maybe log a warning and automatically rename one of the tags... maybe append a "1" to the name or something? I'm not sure how discoverable that would be. Is a user going to find that and realize what happened or is that going to get lost if we do that? Or all user-defined tags get a prefix, that way it's moot. You can name tour tag "dag"and it won't conflict because it gets emitted as "custom_dag" or something like that? Not very elegant, but fairly bombproof if we limit the user's input to only one dict. If we get into trying to merge provided tag dicts then we run into the same issue. For example, if we do this, I like the idea of being able to set tags at various levels. You could set environment-wide tags that get added to everything, and executor-wide tags that gets added to anything run via that executor, etc. It would mean merging dicts at each "level" and the chances of a conflicting tag name increases drastically. We could use a prefix if we did that, indicating the "level" that added the tag... so any tags added to the environment would be "env_foo", and so on.... perhaps harder to find on the user's end, but eliminates the name conflicts. |
Beta Was this translation helpful? Give feedback.
-
|
Yes. You are right. We have to handle all kinds of merge conflicts at multiple levels. We can reserve some keywords for tag keys and use them for out-of-the-box tags. However, customers might still use the custom tag key with different values. In those cases, we can log the warning and say the behavior is undefined. I strongly believe this feature is extremely useful in the case of multitenant airflow use cases. |
Beta Was this translation helpful? Give feedback.
-
I'm definitely not arguing that we can't do it. I do like the idea, but we need to sort out expected behaviors before we dive into it. I think in general the precedent is that the "lower level" would override. So if you have an environment-level and a task-level tag with the same name then it would use the task-level as it's more tightly scoped. By that I mean we assume the user wants the environment-level value used for anything where they didn't specifically set a more granular value. But as I suggested, if we prefix the user-provided keys with the "level" then we can avoid that entirely. Let's use the multitenancy context for an example. Maybe at the environment level they are setting "owner = accounting" and then at the dag level they set "owner = accounts_payable". maybe even an "owner = bob" at the task level so we can track who needs the report that the task generated. I don't know. In that case, with prefixes, any metrics emitted by the task would get the hardcoded default tags plus:
Now the real question is..... is that helping anything? Is my suggestion actually useful or is that over-complicating things and going to cause more confusion than help? Maybe we just need to use strictly the user-provided tag names and figure out a hierarchy. ((pinging @shubham22 and @howardyoo for thoughts)) |
Beta Was this translation helpful? Give feedback.
-
|
If we retain the tags at each level, then it is good and it solves all the merge conflicts. I believe we have two levels, one is default tags and the other is dag level tags. Not sure about task-level tags. |
Beta Was this translation helpful? Give feedback.
-
|
@hussein-awala / @jedcunningham / @potiuk / @kaxil / @eladkal |
Beta Was this translation helpful? Give feedback.
-
|
I believe prefixing with the label on tag names is a good idea. I prefer to
have the delimiter to have '.' instead of '-' (e.g. env.xyz dagrun.xyz
task.xyz) though. Just a preference.
…On Fri, Aug 16, 2024 at 7:57 AM Gopal Dirisala ***@***.***> wrote:
@hussein-awala <https://github.com/hussein-awala> / @jedcunningham
<https://github.com/jedcunningham> / @potiuk <https://github.com/potiuk>
/ @kaxil <https://github.com/kaxil> / @eladkal
<https://github.com/eladkal>
WDYT about this feature in Airflow 3.
—
Reply to this email directly, view it on GitHub
<#36711 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AHZNLLTK2QIFDH5UE4XGMDTZRXZLBAVCNFSM6AAAAABBV7OEUWVHI2DSMVQWIX3LMV43URDJONRXK43TNFXW4Q3PNVWWK3TUHMYTAMZVHA3TQMY>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
Beta Was this translation helpful? Give feedback.
-
Description
With the ability to include tags for both Datadog and Telegraf metrics, it doesn't currently appear possible to include any custom tags defined on the DAG as part of those metrics. Telegraf for instance, supports including arbitrary key/value pairs as part of the metric name, but the set of tags that are included are limited, e.g., TaskInstance includes only
dag_idandtask_idUse case/motivation
We include several metadata tags in our DAGs that we'd like to use as filters within our TIG stack, for example things like environment or stakeholder. One idea would be to modify DAG tags to add support for a dictionary and then use those tags wherever metrics are emitted.
Related issues
No response
Are you willing to submit a PR?
Code of Conduct
Beta Was this translation helpful? Give feedback.
All reactions