howardyoo commented on PR #43340: URL: https://github.com/apache/airflow/pull/43340#issuecomment-2450837709
I generally would not go with using the longer name (even otel has 255 limits to it), simply because of the following: 1. it introduces flood of 'names' in any telemetry data namespace - which makes the management of it a nightmare 2. that was one of the reason why people did not like statsd metrics 3. converting into long names also modifies string (like replacing ' ' with '_' which is fine, but on some conditions, just makes the data messy and 'modified' from its original format, so not a great design. I would still propose when user is switched over the otel metrics, the names would be different (and all those unnecessary part of the name would instead be gone into tags). On Thu, Oct 31, 2024 at 4:08 PM D. Ferruzzi ***@***.***> wrote: > ***@***.**** commented on this pull request. > ------------------------------ > > In airflow/metrics/otel_logger.py > <https://github.com/apache/airflow/pull/43340#discussion_r1825165385>: > > > @@ -300,6 +305,15 @@ def timer( > """Timer context manager returns the duration and can be cancelled.""" > return _OtelTimer(self, stat, tags) > > + def get_name(self, metric_name: str, tags: Attributes | None = None) -> str: > > Cool, maybe it was a bug they fixed on their end. Best way to tell is to > make a metric with a long name and test it out using breeze start-airflow > --integration otel and see if it works. > > If that limit no loner exists then..... I don't know.... I think we should > still use tagging but I don't know what the best approach is for the name > itself. It makes sense to emit the longer name the same as statsd, but then > that's another breaking change since existing otel dashboards would all > break..... I guess that would need a community discussion at that point. > > @ArshiaZr <https://github.com/ArshiaZr> - Next step, please get a breeze > environment running with otel and verify the name limit restriction has > been removed/fixed, then we can go from there. > > @dannyl1u <https://github.com/dannyl1u> is working on the metrics > registry which you are going to use to auto-generate the docs page, so I > guess we need to determine which name we're going to be using before that > gets too far.... > > — > Reply to this email directly, view it on GitHub > <https://github.com/apache/airflow/pull/43340#discussion_r1825165385>, or > unsubscribe > <https://github.com/notifications/unsubscribe-auth/AHZNLLROOXZWX3E6NWMIOBLZ6KL4XAVCNFSM6AAAAABQQJT7UOVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZDIMBZGA3TQNBSHE> > . > You are receiving this because you were mentioned.Message ID: > ***@***.***> > -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
