Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add monitoring with Kamon (disabled by default) #1126
Add monitoring with Kamon (disabled by default) #1126
Changes from 1 commit
ae28445
2d7fb1f
04dc53b
ff317f7
c12064e
312ece8
0d773a2
4b6aa54
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't this be
res onComplete { case Success(_) => timer.stop }
? I'd assume we don't want to mix measurements for failed calls with the successful ones.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We need to stop the timer somehow? I don't think it matters anyway, because Kamon has first-class handling of distribution of latencies, and we would detect outliers easily.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that's where we'd use tags or some equivalent.
Usually we should split our measurements on values of a specific tag with Success, Failure1, Failure2, etc depending on our set of possible failures.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's kamon recommendation for naming spans? Do they automatically included the containing namespace/class/object? It's usually recommended to use some kind of namespacing in spans, so either Kamon does it automatically for us or maybe we should do something like
blockchain.bitcoind.rpc.extendedbitcoinclient.validate
?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's a good point, let's address this in a 2nd iteration
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks like we'd benefit a lot from a utility function in KamonExt to wrap a future in a span.
It would be nice to just write
tx <- KamonExt.wrapInSpan("getrawtx")(getRawTransaction(txid))
which would automatically do thespan.finish()
.The name
wrapInSpan
isn't very good, needs some bikeshedding.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It might be useful to centralize counters for each component (like the
Metrics
object you created forPeer
).For example in Authenticator's companion object, define
private val connectingPeersCounter = Kamon.counter("peers.connecting.count")
.This is usually easier for future maintenance.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is the same counter used in
Authenticator
. So definitely worth centralizing somewhere to make sure these two actors keep using the same counter correctly