-
Notifications
You must be signed in to change notification settings - Fork 808
[ECOINT-248] Redpanda v2.2.0 (metrics update) #2812
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
Conversation
… to not just be -1.
…da.schema_registry.latency_seconds in metadata and testing.
dd-dominic
left a comment
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 fix validations. Once fixed, we'll begin our review.
|
@dd-dominic What do I do about the validations like these? redpanda.cloud.client_lease_duration row has error: Invalid value for According to this, histogram is a supported type |
The in-app type is different. See example here: https://docs.datadoghq.com/metrics/types/?tab=histogram#example. |
|
@dd-dominic Are there restrictions in changing the types listed in metadata.csv? Would we need to publish under a new metric name to change the type? |
You cannot change the metric name, but you can safely change the types. If you need to change the metric name, leave the old metric as is and create a new metric. |
|
@dd-dominic I'm now seeing a bunch of lint issues such as the following: In the source, the line looks like this: If I split it over two lines, the linter combines it back into one, so then the lint check fails since the output of the linter doesn't match the input. What do I need to do here? |
|
@pmw-rp can you confirm the ddev version you're working with ( Additionally, did you try running |
|
Here you go: |
|
@dd-dominic Any progress on the CI issue? |
@pmw-rp I learned we can safely ignore this error for now. Let me know when your changes are complete and will continue the review. |
|
Ok, please continue. |
|
@dd-dominic Just a thought: I could commit local changes to test-target.yml that would 'unpin' the versions, which should unblock the CI problem and allow all of the validations to happen. We'd need to revert that change before merging, but that may make the review process easier? Let me know if you want that and I'll get it done. |
Co-authored-by: Dominic Medina <[email protected]>
Co-authored-by: Dominic Medina <[email protected]>
|
@dd-dominic What's needed to progress here? |
|
@pmw-rp Metric names can't be removed from the metadata.csv file. Can you please double check? Looks like |
|
@pmw-rp let us know when it's ready for another review. Not sure if changes are still being made. As of now, looks like |
|
Ahhh - it looks like this has always been wrong. In metadata.csv, it's referenced as redpanda.cluster.controller_log_limit_requests_dropped, whereas in metrics.py and common.py, it's already referenced as redpanda.controller.log_limit_requests_dropped. I'll push it all back to redpanda.controller.log_limit_requests_dropped, and add an additional line in metadata.csv so that you're not seeing a missing entry. |
|
@dd-dominic Where are we with this? |
|
@pmw-rp working with our team to get this merge this week (hoping today) |
What does this PR do?
This PR brings Redpanda metrics up to date.
Motivation
Completeness, customer demand.
Review checklist
no-changeloglabel attachedAdditional Notes
We will follow with a later PR to add supporting dashboards that demonstrate some of these metrics. The focus here is to enable customers to use metrics that are already available in the product.
This PR replaces the previous PR which was unfortunately closed due to inactivity.
Some of the issues with the previous PR seemed to be related to the DD validation - some help may be required with that.