Skip to content

Add hostname to triggerer metric [triggers.running]#32050

Merged
potiuk merged 7 commits into
apache:mainfrom
apilaskowski:patch-2
Jun 27, 2023
Merged

Add hostname to triggerer metric [triggers.running]#32050
potiuk merged 7 commits into
apache:mainfrom
apilaskowski:patch-2

Conversation

@apilaskowski

@apilaskowski apilaskowski commented Jun 21, 2023

Copy link
Copy Markdown
Contributor

It is impossible to distinguish informations from different triggerers.
We can not approximate the number of triggers that are running at given moment.
Giving hostname suffix will make it possible to approximate this information.

@boring-cyborg boring-cyborg Bot added the area:Scheduler including HA (high availability) scheduler label Jun 21, 2023
@apilaskowski apilaskowski changed the title Add hostname to triggerer metric Add hostname to triggerer metric [trigger.running] Jun 21, 2023
@potiuk

potiuk commented Jun 23, 2023

Copy link
Copy Markdown
Member

Errors. Also maybe @ferruzzi Might have a say after you fix them

ferruzzi
ferruzzi requested changes Jun 26, 2023
View reviewed changes

@ferruzzi ferruzzi left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the tag, Jarek.

Two requests:

  1. The Metrics documentation page is not yet auto-generated, so please update the name there as well (found here). I know it's a little out of date on other names as well, we're working on it.
  2. Please see the suggested change regarding tagging

Comment thread airflow/jobs/triggerer_job_runner.py
@apilaskowski

apilaskowski commented Jun 26, 2023

Copy link
Copy Markdown
Contributor Author

I will update the doc asap (tomorrow morning it is 8pm in Poland right now). Thank you for explaining why we need those tags!

@ferruzzi

ferruzzi commented Jun 26, 2023

Copy link
Copy Markdown
Contributor

I also just noticed I missed a space in the tags dict in my suggestion code, you'll want to run static checks on it and it'll fix that.

@apilaskowski

apilaskowski commented Jun 27, 2023 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

Are those changes related to Core Providers (amazon) problems? @ferruzzi @kosteev
EDIT:
Those test were failing and I wanted to know if it is possible that this changes influenced that (which I suspect is not related).

@apilaskowski
apilaskowski requested a review from potiuk as a code owner June 27, 2023 12:29
@apilaskowski

apilaskowski commented Jun 27, 2023

Copy link
Copy Markdown
Contributor Author

@potiuk @ferruzzi
I updated entry for this metric in documentations, is it correct now?

@apilaskowski
apilaskowski requested a review from ferruzzi June 27, 2023 12:38
@ferruzzi

ferruzzi commented Jun 27, 2023

Copy link
Copy Markdown
Contributor

I think it looks good. The only thing to consider is if this is considered a breaking change. Maybe we should continue emitting the old metric side by side without the hostname? From your initial description it sounds like the original is just broken but can you think of any usecase where someone will be grumpy that we "broke" it?

@apilaskowski

apilaskowski commented Jun 27, 2023

Copy link
Copy Markdown
Contributor Author

It was "broken" in case of multiple Triggerers, so it is possible that someone is using this metric properly for singular Triggerer.

If someone was using this metric previously.
He/she was not using tags for that, so wouldn't this metric with tags reflect the same behaviour (I am not entirely sure)?
I think that tags are opt-in and can be ignored, so this metric should work there (or am I wrong in that regard?).

ferruzzi
ferruzzi approved these changes Jun 27, 2023
View reviewed changes

@ferruzzi ferruzzi left a comment •
edited
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't have a full understanding of Triggerers yet, so I wanted to double check I wasn't missing something there. 👍

Unless someone chimes in otherwise on that point, I'm happy with it.

((@vandonr-amz - Maybe a quick gut check on this?))

vandonr-amz
vandonr-amz approved these changes Jun 27, 2023
View reviewed changes

@vandonr-amz vandonr-amz left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@apilaskowski apilaskowski changed the title Add hostname to triggerer metric [trigger.running] Add hostname to triggerer metric [triggers.running] Jun 27, 2023
@potiuk
potiuk merged commit 215a031 into apache:main Jun 27, 2023
@ephraimbuddy ephraimbuddy added this to the Airflow 2.7.0 milestone Jul 6, 2023
@ephraimbuddy ephraimbuddy added the type:improvement Changelog: Improvements label Jul 6, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@potiuk potiuk potiuk approved these changes
@uranusjr uranusjr uranusjr approved these changes
@ferruzzi ferruzzi ferruzzi approved these changes
@kaxil kaxil Awaiting requested review from kaxil
@ashb ashb Awaiting requested review from ashb
@XD-DENG XD-DENG Awaiting requested review from XD-DENG
+1 more reviewer
@vandonr-amz vandonr-amz vandonr-amz approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

area:Scheduler including HA (high availability) scheduler type:improvement Changelog: Improvements

Projects

None yet

Milestone

Airflow 2.7.0

Development

Successfully merging this pull request may close these issues.

6 participants

@apilaskowski @potiuk @ferruzzi @uranusjr @vandonr-amz @ephraimbuddy