Add first opentelemetry metric #393
Conversation
|
@erebe first draft PR for the metrics provider. This is mostly to make sure the general style fits your expectations, though could already be merged as is. This is the ~MVP of getting some prometheus metrics with the opentelemetry tooling. The main potentially contentious point I see: This tooling is quite beta/alpha stage in the rust ecosystem. |
| self.metrics.connections.add( | ||
| 1, | ||
| &[ | ||
| KeyValue::new("remote_host", format!("{}", remote.host)), |
There was a problem hiding this comment.
We should avoid setting remote_host and port as label as it will make too much cardinality for prometheus, and create a data leak for us.
If a user request random host / port our metrics are going to fill the available memory of wstunnel until it OOM as they are never cleaned.
metrics should not jeopardize the stability of the system
https://stackoverflow.com/a/69167162
There was a problem hiding this comment.
I'll have to think about this a bit.
I see why you say that, and agree with the point you make.
OTOH, I need the cardinality to get the information I need. So I'll have to think about some way to customize this.
In my case, k8s and multiple replicas make an OOM less serious.
|
Overall, looks ok 👍 |
Ongy
force-pushed
the
add-basic-metrics
branch
from
79824d2 to
ae52331
Compare
Ongy
force-pushed
the
add-basic-metrics
branch
2 times, most recently
from
a3375f3 to
d065bfa
Compare
No description provided.