feat: add denylist to stats logger #25118
Conversation
|
Ping @zephyring |
| You can also pass metric names that should be ignored via an optional `metric_denylist` parameter: | ||
|
|
||
| ``` | ||
| STATS_LOGGER = StatsdStatsLogger(host='localhost', port=8125, prefix='superset', metric_denylist={"reports.scheduler"}) |
There was a problem hiding this comment.
I wonder if it's better to promote metrics_denylist to be a config that can be overridden.
There was a problem hiding this comment.
something like
| STATS_LOGGER = StatsdStatsLogger(host='localhost', port=8125, prefix='superset', metric_denylist={"reports.scheduler"}) | |
| STATS_LOGGER_METRICS_DENYLIST: Set[str] = {} | |
| STATS_LOGGER = StatsdStatsLogger(host='localhost', port=8125, prefix='superset', metrics_denylist=STATS_LOGGER_METRICS_DENYLIST) |
There was a problem hiding this comment.
Wouldn't that be the same as just overwriting the STATS_LOGGER with different arguments?
There was a problem hiding this comment.
@zephyring or @craig-rueda mentioned a good point that there's no guarantee that a metric name might not change. What do you all think about having an enum of strings that we can reference instead? It will have to be maintained and updated of course, but at least then we can export it and refer to the enum in the config rather than the string.
There was a problem hiding this comment.
If promoting to be a config, we can just override the config and use the default StatsdStatsLogger logger.
If later on there's any default in the denylist that will be easier to reference and merge/override also
| + f"{value}" | ||
| + Style.RESET_ALL | ||
| ) | ||
| if self.should_log(key): |
There was a problem hiding this comment.
@villebro so anyone who wants to use this denylist is going to have to implement the functionality in their own statslogger if they don't use the dummy. Should we put something in UPDATING to explain how to do this or do you think it's evident enough when reading this local example?
There was a problem hiding this comment.
You're right, maybe we should just add it to StatsdStatsLogger, and remove it from BaseStatsLogger. This will cover the vanilla StatsD use case, and should serve as an example for anyone who wants similar functionality in their own custom logger class. Thoughts @eschutho
There was a problem hiding this comment.
Yeah, that sounds like a good idea.
There was a problem hiding this comment.
+1 to comments above.
I don't find much value in having the deny list and find def should_log sufficient for filtering and good enough example on how to define a custom logger with filtering.
But that's just a minor preference and current solution is fine by me as well
|
Heya... it seems like this PR hasn't been rebased or touched in quite a while. Do we still want to keep it open? |
|
I'm assuming this is no longer needed, so closing it out. |
SUMMARY
This is an alternative solution to #25104 and a follow-up to #25080 that makes it possible to ignore logging of certain metrics.
BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
TESTING INSTRUCTIONS
ADDITIONAL INFORMATION