Idiomatic log metrics with customizable naming #597
Conversation
987Nabil
changed the title
987Nabil
force-pushed
the
custom-metric-names
branch
from
6e6b347 to
eda55c2
Compare
987Nabil
changed the title
|
hi @987Nabil, thank you for contribution, in my opinion is hard to say what is idiomatic in this case, in various systems/frameworks/applications both solution are used:
i think it it's about preferences in terms of options (of course sometimes one solution may be better then other ...) originally i made metric per log level, to have similar style like in zio core (there are specific metrics, for zio_fiber_started, zio_fiber_successes, zio_fiber_failures), basically in original PR (#590) i mentioned also alternative solution which is same like here in your PR but yes, it is easier to change naming in case of common metric with tags personally I am ok with your proposal, more like there may be question in relation to backward compatibility, because this change is not backward compatible |
docs/metrics.md
Outdated
| * `LogLevel.All` -> `all` | ||
| * `LogLevel.Fatal` -> `fatal` | ||
| * `LogLevel.Error` -> `error` | ||
| * `LogLevel.Warning` -> `warning` |
There was a problem hiding this comment.
label for LogLevel.Warning is WARN
then this should be:
LogLevel.Warning -> warn
|
@justcoon I think Scala api wise it is. Only the name of the metric is a problem. But this is out now for 2 days? All places I came a long sofar prefer labels. |
docs/metrics.md
Outdated
| * `LogLevel.Info` -> `info` | ||
| * `LogLevel.Debug` -> `debug` | ||
| * `LogLevel.Trace` -> `trace` | ||
| * `LogLevel.None` -> `none` |
There was a problem hiding this comment.
label for LogLevel.Warning is OFF
then this should be:
LogLevel.None -> off
NOTE: here is bug also in original documentation
|
ok, that is true, that it is released just few days (and also there are holidays now) i added some comments, also code needs to be reformatted (build failing on scalafmtCheck - there is |
|
Always the formatter. I am used to formatting that is applied and pushed by the pipeline. But I think this only works for private repos. |
987Nabil
force-pushed
the
custom-metric-names
branch
from
eda55c2 to
922d030
Compare
| } yield ExitCode.success) | ||
| .provideLayer((ZLayer.succeed(MetricsConfig(200.millis)) ++ publisherLayer) >+> prometheusLayer) | ||
|
|
||
| } | ||
| ``` | ||
|
|
||
| Expected Console Output: | ||
|
|
There was a problem hiding this comment.
in relation to metric changes, console output for this example probably also changed,
i think you should run MetricsApp from examples and update it here
The current impl. creates many metrics, which is not idiomatic. They are all the same metric with different notions. Therefore I propose to use labels instead with only one counter.
Also I think being forced to use a certain metric name is not good, since orgs might enforce naming conventions.
I remember, that we have the option for custom naming in other zio metrics too. I think it was zio-http.