-
Notifications
You must be signed in to change notification settings - Fork 306
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
flink: add metrics support #2633
Conversation
60e9485
to
9cf73e7
Compare
@mobuchowski Thanks for adding the metrics reporting functionality. Would you please add some context in README about how users should use and interpret the metrics? |
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 provide description of this PR of what it adds and specify what metrics will be produced. This super helpful when trying to figure what was changed in the past by a PR.
Documentation PR is important as well:
- There is metrics section in Java client, however it is not linked from Spark integration. I think it should be included in both Spark and Flink integrations (include md partial)
- Additionally, I think it's important to provide info specific to Spark and Flink. As a Spark/Flink user I would like to know which metrics are emitted just to know if this is going to solve the problem I have or not.
"https://github.com/OpenLineage/OpenLineage/tree/%s/integration/flink", getVersion())); | ||
} | ||
|
||
@SuppressWarnings("PMD") |
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 mention particular warning you want to suppress.
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.
@pawel-big-lebowski to be honest, I've copied this fragment from Spark integration. Should I change this there as well?
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.
It's still in some places for Spark. However, in most places we're just suppressing a rule that is found problematic like:
@SuppressWarnings("PMD.AvoidCatchingNPE")
It's a better approach I think.
Signed-off-by: Maciej Obuchowski <obuchowski.maciej@gmail.com>
9cf73e7
to
3567f70
Compare
Signed-off-by: Maciej Obuchowski <obuchowski.maciej@gmail.com>
This adds support for #2496 metrics for Flink integration.