Skip to content
This repository was archived by the owner on Aug 2, 2021. It is now read-only.

all: change metric names delimiter . to / (#2138) #2139

Merged
merged 9 commits into from
Mar 25, 2020
Merged

Conversation

vandot
Copy link

@vandot vandot commented Mar 16, 2020

All metric name delimiters are changed except for:
all runtime metrics from debug/api and
https://github.com/ethersphere/swarm/blob/metric-names-rewrite/p2p/protocols/protocol.go#L285 %T is the problem

@vandot vandot requested review from janos and acud March 16, 2020 15:17
@vandot vandot linked an issue Mar 16, 2020 that may be closed by this pull request
Copy link
Member

@janos janos left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks Ivan. Looks OK, I could not find a miss except in no-code README. https://github.com/ethersphere/swarm/blob/master/README.md#metrics-and-instrumentation-in-swarm.

  1. https://github.com/ethersphere/swarm/blob/metric-names-rewrite/p2p/protocols/protocol.go#L285 %T is the problem

It is possible to use strings.ReplaceAll to replace all problematic characters? Probably to replace . with _.

  1. runtime metrics

The problem is in a vendored ethereum/go-ethereum https://github.com/ethersphere/swarm/blob/metric-names-rewrite/vendor/github.com/ethereum/go-ethereum/metrics/runtime.go#L57.

I think that it would be most reliable just to copy code that does runtime metrics in go-ethereum/metrics into swarm/metrics package and fix the naming problem. We are calling two functions from there RegisterRuntimeMemStats and CaptureRuntimeMemStats. These functions and all of their required unexported variables, types and functions can be just copied and fixed. This would be much easier then to bother fixing this issue upstream and dealing with go-etehereum dependency version upgrade.

We are lucky not to use RegisterDebugGCStats from https://github.com/ethersphere/swarm/blob/metric-names-rewrite/vendor/github.com/ethereum/go-ethereum/metrics/debug.go#L64.

Is it ok for you to fix the rest of the problematic metrics with my suggestions?

@vandot
Copy link
Author

vandot commented Mar 16, 2020

@janos fixed peer/send metric delimiter as proposed with strings.ReplaceAll but stucked with runtime.go

@janos
Copy link
Member

janos commented Mar 19, 2020

TestReporter in github.com/ethersphere/swarm/p2p/protocols is failing because accounting metrics are persisted and the keys in the database are not updated. They are defined in p2p/protocols/reporter.go metricsMap.

I do not think that metrics should be used for anything else except instrumentation and that accounting should not rely on metrics in order to function well. Regardless of that, this is implemented and it should work.

@vandot vandot merged commit 17a389d into master Mar 25, 2020
@vandot vandot deleted the metric-names-rewrite branch March 25, 2020 16:03
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

rename metric names
3 participants