ferruzzi commented on PR #69371: URL: https://github.com/apache/airflow/pull/69371#issuecomment-4919109814
Thanks for writing this. To be honest, I can't believe I didn't include it in the docs. A couple of thoughts, since you are in there: One thing that I've noticed catches people up is that `metrics_allow_list` is exhaustive. If your admin uses `metrics_allow_list` and your custom metric isn't on it, it gets silently swallowed and this has confused folks in the past. It may be worth a line to make it extra explicit. Since you are adding this example and documentation, is it maybe worth including more detail about the available parameters for each? For example, `incr` and `decr` both accept `count` and `rate`, etc? Or do we trust them to just find the relevant classes and read the code? I'm fine either way, just thinking. I think one important detail that you missed is metric name constraints. Names must be <= 250 chars and only contain [a-zA-Z0-9_.-/]. Invalid names raise `InvalidStatsNameException` which gets caught and logged, but the metric is not emitted. Users should know the rules. And, finally, can you run the build-docs and make sure those cross-links work int he final rendered version? `rst` linking can be fiddly. -- 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]
