-
Notifications
You must be signed in to change notification settings - Fork 456
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
[Airflow] Revert metrics field definition to the format used before i… #7469
Conversation
…ntroducing metric_type
🌐 Coverage report
|
Why the reverting is done? Can you link to the related issue? |
This is the issue link |
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!
Package airflow - 0.3.0 containing this change is available at https://epr.elastic.co/search?package=airflow |
@@ -435,7 +435,7 @@ | |||
"dataType": "number", | |||
"isBucketed": false, | |||
"label": "Scheduler Heartbeat", | |||
"operationType": "sum", | |||
"operationType": "max", |
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.
@ritalwar, Was this an aggregation change to fix some issue?
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.
This was intended for the dashboard, as the sum aggregator isn't compatible with counters.
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.
avg() replaced by max() is understandable. But, sum() replaced by a max(), might change what value is shown by the dashboard panel. Can you re-assess the correctness of the dashboard value following this change?
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.
Please create an issue for tracking purpose, if needed.
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.
Agree, we can summarise the change in a top level issue (existing Airlfow TSDB issue also works)
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.
Created this issue.
What does this PR do?
Revert airflow metrics field definition to the format used before introducing metric_type.
This is done because of this issue.
Although, Airflow remained unaffected by this issue. However, with the fix now accessible and the necessary changes present in the elastic-package, it's good to revert to the original format.
Checklist
changelog.yml
file.Related issues
Screenshots