Implemented get_name in StatsLogger, updated Otel and StatsD #43340
Conversation
- Added get_name method in StatsLogger (base_stat_logger.py) to handle metric name preparation and tag validation. - Updated Otel and StatsD loggers to inherit from StatsLogger and utilize the new get_name method. - Refactored tag preparation and validation logic into get_name for a cleaner and more consistent implementation.
|
@ferruzzi, @howardyoo, @o-nikolas Can you please take a look at it? If anything needs to change please let me know. |
@ArshiaZr, I am not a reviewer, and thus cannot review this PR, but I believe it would be nice if you can add unit tests for your changes as part of this PR, so that the changes can be properly checked to work properly. cc to @ferruzzi |
Actually - anyone can review PRs - and we encourage it. You can even approve it @howardyoo and it might guide other maintainers with their approvals. |
|
No, as far as I know, I don’t think I have the ability to approve the PR, but if I get a chance I’ll sure do the review. Having the unit test would be very helpful for the testing too.Sent from my iPhoneOn Oct 25, 2024, at 12:11 PM, Jarek Potiuk ***@***.***> wrote:
@ArshiaZr, I am not a reviewer, and thus cannot review this PR, but I believe it would be nice if you can add unit tests for your changes as part of this PR, so that the changes can be properly checked to work properly. cc to @ferruzzi
Actually - anyone can review PRs - and we encourage it. You can even approve it @howardyoo and it might guide other maintainers with their approvals.
—Reply to this email directly, view it on GitHub, or unsubscribe.You are receiving this because you were mentioned.Message ID: ***@***.***>
|
You can approve it (many contributors do that). But you need at least one approval from a commiter to merge it - but approvals can be done by absolutely anyone who has a GitHub account. |
|
Ok, I see. Thanks!Sent from my iPhoneOn Oct 25, 2024, at 12:26 PM, Jarek Potiuk ***@***.***> wrote:
No, as far as I know, I don’t think I have the ability to approve the PR
You can approve it (many contributors do that). But you need at least one approval from a commiter to merge it - but approvals can be done by absolutely anyone who has a GitHub account.
—Reply to this email directly, view it on GitHub, or unsubscribe.You are receiving this because you were mentioned.Message ID: ***@***.***>
|
|
I reviewed the code, and to me, it is well implemented and ready to be
merged.
…On Fri, Oct 25, 2024 at 12:30 PM Howard Yoo ***@***.***> wrote:
Ok, I see. Thanks!
Sent from my iPhone
On Oct 25, 2024, at 12:26 PM, Jarek Potiuk ***@***.***>
wrote:
No, as far as I know, I don’t think I have the ability to approve the PR
You can approve it (many contributors do that). But you need at least one
approval from a commiter to merge it - but approvals can be done by
absolutely anyone who has a GitHub account.
—
Reply to this email directly, view it on GitHub
<#43340 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AHZNLLU25JM2URXAA4NZZRLZ5J5M7AVCNFSM6AAAAABQQJT7UOVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDIMZYGQYDEMRYGA>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
|
Do we expect any incompatibilities in produced metrics @ArshiaZr ? I would love to understand what was the intention of "cleaner and more consistent implementation." before I get into details. |
…tsdLogger for enhanced validation - Modified StatsLogger base class to enforce stricter validation on metric names and prefixes, ensuring all names are strings and conform to required formats. - Enhanced SafeOtelLogger with additional validation to support OpenTelemetry standards and added regression tests for metrics with and without tags. - Updated SafeStatsdLogger to align with new validation standards and to handle edge cases more robustly. - Added regression tests to catch issues with missing tags, empty names, and overly long names, ensuring consistency across changes.
|
I just made the code more robust based on @ferruzzi reviews I'd appreciate it if you could look at it. cc @howardyoo, @dannyl1u |
|
@ferruzzi Thanks for the reviews. Here's the fixed version. I had to add the instance back in StatsLogger as I noticed we're using it somewhere else. |
|
I raised another commit. I tried to address all the issues and reviewed it myself multiple times. |
Thanks @howardyoo
@ashb I concur. Can we agree that the Otel names should continue to be the name without the embedded variables, as it is now, and (assuming Howard confirms the external verification is actually working) just remove our truncating and validating the name?
I'm not very familiar with datadog. Looking at the existing code, its get_name implementation should be similar to OTel's and just pass the name through as provided since it also gets the tags added later, right? @ArshiaZr - Please implement the datadog part and either you and/or Howard can sort out what is going on with the otel name validation after that. |
|
Yes, we shouldn’t have the limit to 32 chars - that is way too small and not useful.
|
- Added prepare_stat_with_tags decorator to standardize tag processing across all metrics functions. - Ensured tags default to an empty list [] when no valid tags are provided or self.metrics_tags is disabled, matching expected behavior in tests. - Updated all metric functions (e.g., incr, decr, gauge, timing, timer) to use the new decorator for consistent tag validation and formatting.
Rationale for Using Decorators Instead of Inheritance with DatadogUsing Even if tags need to be prepared, we already apply the Therefore, making @ashb @ferruzzi if you have any other suggestions just let me know |
|
Not being a subclass sounds fine as long as it follows the same interface/protocol. I'll check out the changes when I can. |
|
We discussed on slack, but I'll also reply here for posterity: I think inheritance is the right way to go here. Should someone make a change tot he base class in the future, it would be confusing to troubleshoot why StatsD and OTel both work but Datadog doesn't. So it should inherit for the sake of consistency, if nothing else. |
|
@ashb - Can we get this merged? |
|
Looking at this right now, sorry for the delay |
This review actioned, new changes but less blocking now
|
@ArshiaZr - You can also remove the portion(s) of code that limit the OTel name length now. I think we've all agreed that we can keep the names short as Best Practice but should no longer be truncating them. |
…apply minor refinements
| if len(proposed_stat_name) > OTEL_NAME_MAX_LENGTH: | ||
| # If the name is in the exceptions list, do not fail it for being too long. | ||
| # It may still be deemed invalid for other reasons below. | ||
| for exemption in BACK_COMPAT_METRIC_NAMES: |
There was a problem hiding this comment.
My IDE is currently borked so I can't double check this easily; is BACK_COMPAT_METRIC_NAMES used anywhere else? It's possible that can also be pruned out now if it was only ever used as a work-around for the name length bug.
|
They increased the name length in September of last year 😅 open-telemetry/opentelemetry-python#3442 |
^ Add meaningful description above
Read the Pull Request Guidelines for more information.
In case of fundamental code changes, an Airflow Improvement Proposal (AIP) is needed.
In case of a new dependency, check compliance with the ASF 3rd Party License Policy.
In case of backwards incompatible changes please leave a note in a newsfragment file, named
{pr_number}.significant.rstor{issue_number}.significant.rst, in newsfragments.