Add OpenTelemetry tracing instrumentation #1199

dlsniper · 2023-03-07T18:36:24Z

This commit adds instrumentation to Drand.
It also adds the required development instructions for it.

Fixes #632.

chain/beacon/chainstore.go

test/metrics.go

test/docker-compose.yaml

metrics/tracer.go

docker-compose.yaml

devenv/docker-compose.yaml

dlsniper

I've addressed the code review comments, please take a look.

chain/beacon/chainstore.go

devenv/docker-compose.yaml

docker-compose.yaml

metrics/tracer.go

test/docker-compose.yaml

test/metrics.go

CluEleSsUK

A few bits and bobs, and questions. It could be nice in future to combine logging and telemetry code into one abstraction, although it's very late in the day to be changing that (and some weird external services are requiring on our log messages so might cause some hiccups...).

Other than the few small things I caught, lgtm!

chain/beacon/chainstore.go

chain/beacon/node.go

CluEleSsUK · 2023-03-31T08:36:06Z

chain/beacon/sync_manager.go

-				ctx, cancel = context.WithCancel(context.Background())
-				//nolint
+				ctx, cancel = context.WithCancel(s.ctx)
+				//nolint:errcheck // TODO: Handle this


don't forget to handle this!

We need to discuss about this. The reason for this is, the call is async. We should rewrite most of the logic there to:

first wait for the existing s.Sync() call to finish after a cancel is issued

then, start a new s.Sync().

finally, if an error happens, we need to determine how to handle the error: just log it, abort the SyncManager, or something else?

That's a good point, we should wait for the previous one to finish. But that should be fairly quick I guess, since we can close the pipes from our side directly.

Errors in the Sync manager should only be logged but not abort the whole thing. We could have an exponential cool-off period to avoid DOSing our node while trying to query the other ones in case of an issue with all other nodes I guess.

cmd/drand-cli/cli_test.go

cmd/drand-cli/cli.go

core/drand_daemon_control.go

dkg/actions_passive.go

dlsniper · 2023-03-31T14:45:50Z

It could be nice in future to combine logging and telemetry code into one abstraction

I agree, it's a good idea. We can do this after the PR moving everything to internal.

although it's very late in the day to be changing that (and some weird external services are requiring on our log messages so might cause some hiccups...).

I'm not sure I follow this. Are you saying that the error messages should not be changed?

Initial commit for adding OT tracing using a customizable endpoint

* OT tracing using a customizable endpoint --------- Co-authored-by: Patrick McClurg <patrick.mcclurg@protocol.ai> Co-authored-by: PM <3749956+CluEleSsUK@users.noreply.github.com>

AnomalRoil

Alright, caught up on that review. Will address my own comments in a follow-up PR :D

AnomalRoil · 2023-05-05T15:21:05Z

chain/beacon/sync_manager.go

@@ -109,12 +114,17 @@ type RequestInfo struct {
 // round. Depending on the current state of the syncing process, there might not
 // be a new process starting (for example if we already have the round
 // requested). upTo == 0 means the syncing process goes on forever.
-func (s *SyncManager) SendSyncRequest(upTo uint64, nodes []net.Peer) {
-	s.newReq <- NewRequestInfo(upTo, nodes)
+func (s *SyncManager) SendSyncRequest(spanContext oteltrace.SpanContext, upTo uint64, nodes []net.Peer) {


I'm really not too fond of passing oteltrace.SpanContext instead of raw context.Context around :/

AnomalRoil · 2023-05-05T15:21:24Z

chain/beacon/sync_manager.go

 }

 type RequestInfo struct {
+	spanContext oteltrace.SpanContext


Not too fond of not having a context.Context instead.

AnomalRoil · 2023-05-05T15:21:35Z

chain/beacon/sync_manager.go

 }

-func NewRequestInfo(upTo uint64, nodes []net.Peer) RequestInfo {
-	return RequestInfo{upTo: upTo, nodes: nodes}
+func NewRequestInfo(spanContext oteltrace.SpanContext, upTo uint64, nodes []net.Peer) RequestInfo {


AnomalRoil · 2023-05-08T09:45:21Z

cmd/relay/main.go

+
+var tracesProbabilityFlag = &cli.Float64Flag{
+	Name:    "traces-probability",
+	Usage:   "Publish metrics to the specific OpenTelemetry compatible host:port server.",


Seems like the wrong Usage text

AnomalRoil · 2023-05-08T09:48:24Z

cmd/relay/main.go

 // Relay a GRPC connection to an HTTP server.
 //
 //nolint:gocyclo,funlen
 func Relay(c *cli.Context) error {
+	tracesProbability := 0.1


Not sure why we use a different value from the default one here.

* fixing belated code review comments about 1199 * callback now has access to bp.log * replacing otel.SpanContext with plain Context * using new context for DKG update listeners * using new context for broadcast * using new context for sender run

* OT tracing using a customizable endpoint --------- Co-authored-by: Patrick McClurg <patrick.mcclurg@protocol.ai> Co-authored-by: PM <3749956+CluEleSsUK@users.noreply.github.com>

* fixing belated code review comments about 1199 * callback now has access to bp.log * replacing otel.SpanContext with plain Context * using new context for DKG update listeners * using new context for broadcast * using new context for sender run

* Updating libp2p again * Make sure version is compat with v2 DKG refactor -> develop to unblock Florin (#1200) - moved dkg package from core to root - re-added demo project using the new DKG - fixed first round of PR comments - refactored test objects for easier readability in tests using alice, bob, carol - extracted some action utilities and add helpful comments - PrivateGateway now implements a DKG client for TLS - added transition time flag to the DKG CLI - fixed a lot of small bugs - dkg kickoff grace period is now a daemon param and is reduced in the test code - added stop check to broadcaster - moved stopped into echoBroadcast itself - DKG requests are now sent in parallel to speed up execution - use the internal clock correctly in the beacon handler Final round of tests improvements (#1187) * Test improvements * Fix connectivity test * Fix test after rebase * Revert to computed round in test * Do not use a package level variable for command output * Allow node status to reply regardless of DKG state or params * We expect at least the previous round to be caught up by now * The sync mechanism will balance out and resolve the discrepancy, eventually. * Speed up the orchestration tests shutdown * A bit more sleep in test test with retrive old beacon from new node (#761) * test with retrive old beacon from new node * Update to latest code changes --------- Co-authored-by: Florin Pățan <florinpatan@gmail.com> Handle error in test (#1212) * Handle error in test Fix typo in vault (#1213) * Fix typo in vault Allow logs to propagate to the test name (#1114) * Make logger propagate across whole codebase * Remove sleep in tests * Remove deprecated sleep function * Fix missing cancel function missing call DKG migration tool from v1.* to v2.0.0 (#1215) * DKG migration tool from v1.* to v2.0.0 * implemented the migration * added a CLI command for running it automagically for each beaconID * reload beacon upon migration * added integration test for restoring node state from migration run tests on develop (#1216) improved output for DKG status command (#1218) - fixed a bug where the transition time was 0 for initial DKG - increased genesis time for tests due to the memdb test getting some weird sync deadlock Add OpenTelemetry tracing instrumentation (#1199) * OT tracing using a customizable endpoint --------- Co-authored-by: Patrick McClurg <patrick.mcclurg@protocol.ai> Co-authored-by: PM <3749956+CluEleSsUK@users.noreply.github.com> show DKG status string instead of iota (#1221) Removing redundant Expires header from server (#1191) * Removing redundant Expires header from server If there is a Cache-Control header with the max-age or s-maxage directive in the response, the Expires header is ignored. Also reworking a few Cache-control max-age directives Move to internal (#1214) * Add OpenTelemetry tracing instrumentation Initial commit for adding OT tracing using a customizable endpoint Instrument the DKG code Move everything to internal Upgrade golangci-lint to 1.50.2 and correctly configure the imports linter Allow Docker to build with the new paths Align Go versions to 1.19.5 in both CI and Docker Expose types to allow for Gossip client demo. Drop WithLogger usage More code cleanup. Stop using panic in tests Allow clients to be built based on Lotus example and demo code moved tracing docker-compose * added details of tracing to the readme * fixed import error * extracted `crypto` module to top level * ran GCI --------- Co-authored-by: Patrick McClurg <patrick.mcclurg@protocol.ai> cherry-picked commits from master (#1235) * threshold monitoring for beacon processes (#1220) * threshold monitoring for beacon processes beacon processes now log some error and warning messages whenever we get close to or cross a threshold number of failures per node * return on finished threshold monitor Co-authored-by: Yolan Romailler <AnomalRoil@users.noreply.github.com> * added prometheus metric for failing to send partials (#1223) * added prometheus metric for failing to send partials * moved import around * fixed tests * made sleeps in the publicrandstream variable - upped local sleep slightly * aggregate prometheus partial errors by beaconID (#1232) The additional fields in the partial error counter meant that a new counter was created for _every_ combination. Given that they could only be emitted for each round, a new entry was created for every partial... which is definitely wrong * use a gauge for counting nodes that have invalid partials (#1233) * use a gauge for counting nodes that have invalid partials strictly speaking this will lose sight of nodes that go down and back up between prometheus polls; however it's much easier to handle in grafana * combined threshold monitor and prometheus call * return 404 when no beacon hash exists instead of 500 (#1234) * return 404 when no beacon hash exists instead of 500 * add test for 404 on nonexistent chain * cherry-picked master commits and fixed a few bits --------- Co-authored-by: Yolan Romailler <AnomalRoil@users.noreply.github.com> Co-authored-by: Houlton McGuinn <hoult.mcguinn@gmail.com> Addressing comments for #1199 (#1236) * fixing belated code review comments about 1199 * callback now has access to bp.log * replacing otel.SpanContext with plain Context * using new context for DKG update listeners * using new context for broadcast * using new context for sender run Updating our dependencies to support Go 1.20 (#1238) * Updating our dependencies to support Go 1.20 This also allows us to finally update to the latest grpc since our blocking dependencies got updated too. * Updating to protoc 3.20.3 * updating to golangci-lint 1.52.2 * updating to latest golangci * Not on my watch Automate DKG migration on startup (#1225) * automate DKG migration on node startup handle no such file or directory error as expected fixed the workflow a little to ensure no nil references check no such group file error specifically DKG messages between participants are now signed and verified (#1229) Miscellaneous fixes from merged reviews (#1237) * added lots of comments to the state machine * don't consume genesis beacon from others * make transition time mandatory to CLI * added a test around pretty printing * added missing flag to demo project * added missing tests all DKG packets are now gossiped throughout the network (#1230) * DKG messages between participants are now signed and verified * all DKG packets are now gossiped throughout the network * DKG RPCs are split into `Command`s and `GossipPacket`s * the flow of execution is now a lot simpler and with fewer higher-order-function shenanigans * remove the unnecessary (racy) brokenbroadcaster * updated with the rebased signatures branch * additional nil checks * added DKG failed state in for DKGs that don't hit threshold * use beaconID from metadata rather than passing it around * added a timeout for the large DKG * removed unnecessary grpc call options * Update internal/dkg/actions.go Co-authored-by: Yolan Romailler <AnomalRoil@users.noreply.github.com> * use mock clock for determining all DKG timings * fixed some references to clocks * fixed an old test where multiple clocks were being created --------- Co-authored-by: Yolan Romailler <AnomalRoil@users.noreply.github.com> DKG refactor: use existing keys to generate proposal (#1253) * use existing keys when generating proposal files * fixed an encoding test * copy instead of pointer to defeat the dreaded race detector * rebase + errors.Is Don't use more than 1 handler (#1260) * Don't use more than 1 handler * short tests must be quick Cherry-pick master ontop of develop (#1256) * added prometheus metric for failing to send partials * moved import around * fixed tests * made sleeps in the publicrandstream variable - upped local sleep slightly aggregate prometheus partial errors by beaconID (#1232) The additional fields in the partial error counter meant that a new counter was created for _every_ combination. Given that they could only be emitted for each round, a new entry was created for every partial... which is definitely wrong use a gauge for counting nodes that have invalid partials (#1233) * use a gauge for counting nodes that have invalid partials strictly speaking this will lose sight of nodes that go down and back up between prometheus polls; however it's much easier to handle in grafana * combined threshold monitor and prometheus call return 404 when no beacon hash exists instead of 500 (#1234) * return 404 when no beacon hash exists instead of 500 * add test for 404 on nonexistent chain bumped patch version for testnet release (#1239) fixed a bug where docker image is pushed with invalid tag (#1240) fixed docker publish action to publish to GHCR (#1241) * fixed docker publish action to publish to GHCR added reference to GCHR in docker publish (#1242) updated docker entrypoint permissions so the container starts (#1243) Update go-libp2p to v0.27.3 (#1244) Updating our dependencies (#1246) * Updating our dependencies for mainnet release * Bumping version to v1.5.6 upgrade libp2p to v0.27.5 (#1247) Fixing wrong logging levels when checking past beacons with sync and shorter tests (#1248) * Fixing wrong logging levels when checking past beacons with sync * Skipping slow tests in short mode Adding a G1 scheme that's RFC conformant (#1249) * Adding a G1 scheme that's RFC conformant * Using Go 1.20 in GHA * patching GHA to support new RFC conformant scheme added new flag for backup out with better text (#1250) updated dockerfiles to use go 1.20 (#1255) * updated dockerfiles to use go 1.20 - added a github action step for building the docker image on branches to ensure compat * renamed the docker build task * added branches for push * run on all pull requests patching updating deps and patching last details More slow test remove from short improving test logging to debug improving TestBeaconSync fixing TestDrandPublicRand fixing test trying to avoid unfinished sync avoiding race in DKG fixing weird build issue adding TestBeaconSync to slow test improve logging tag slow test removing noisy log Adding scheme to self-signature of key Also adding keys signature validation to DKG proposal validation adding missing imports using proper scheme in tests properly close control client in cli Hopefully avoiding the control tcp issues avoid closing conn always cancelling the stream ctx correctly do not close conn increased sleep time when waiting for clock changed DKG runner status check to not optimistically return an error Co-authored-by: PM <3749956+CluEleSsUK@users.noreply.github.com> demo bug fixes; execution kickoff alignment (#1263) * fixed error generating first proposal and panic when viewing DKG state * all nodes now share a kickoff time set by the leader, rather than simply waiting 5mins parallelise state machine tests (#1269) separate initial DKG and resharing commands (#1264) * separate initial DKG and resharing commands * time -> delay Tear-out of client and relay code. (#1265) * Initial tear-out. Working daemon, failing tests * Removed builtin TLS * removed unused initial stores from daemon * exposing the time of round APIs * Improving version handling * migration path self-sign * simplify version handling in v2, we keep retro-compat with 1.5.7+ and 2.x-1.y * Fixing metrics * Leaner logs, fixing wait in test Cherry picking typos corrections (#1274) * cherry picking typo corr * Correcting comment --------- Co-authored-by: Alejandro Criado-Pérez <alejandro@criadoperez.com> Properly process all beacons when self-signing (#1275) Merging version changes from master to v2 (#1276) migration path for pub keys (#1277) * added a migration path for new public key format * removed some TLS mentions * added test for migration path and fixed some errors * PR comments and fixed generated protobuf * fixed some linter rules and turned off depguard * modified catchup period (and renamed in places) re-add tls for keys (#1281) * re-added TLS to identities/participants * fixed some linter complaints * fixed some tests * remove TLS from check and follow chains * add TLS/insecure to remote-status stuff * Bringing back TLS keys * added missing TLS to TomlParticipant * use insecure keys for more testing instances --------- Co-authored-by: Yolan Romailler <yolan@protocol.ai> Exposing the test/mock package (#1278) * Exposing the test/mock package * Making sure we are using the expected output format More specifically chain.Beacon doesn't have the same tags for JSON formatting as client.RandomData did and so it wasn't being marshaled as it used to when we used the asRD and the RandomData struct in our client code for the HTTP server * Updating to latest x/net dependencies * fixing orchestrator as per review chore(docs): update TOC addressed merge comments from PR readded some insecures in docker updated some docker image versions seal of approval

* OT tracing using a customizable endpoint --------- Co-authored-by: Patrick McClurg <patrick.mcclurg@protocol.ai> Co-authored-by: PM <3749956+CluEleSsUK@users.noreply.github.com>

* fixing belated code review comments about 1199 * callback now has access to bp.log * replacing otel.SpanContext with plain Context * using new context for DKG update listeners * using new context for broadcast * using new context for sender run

dlsniper changed the base branch from develop to feature/dkg-refactor March 8, 2023 14:41

dlsniper changed the base branch from feature/dkg-refactor to develop March 8, 2023 14:50

AnomalRoil force-pushed the develop branch from 555b797 to 36a7076 Compare March 21, 2023 12:27

dlsniper force-pushed the instrumentation branch from 73e236f to f013f27 Compare March 24, 2023 17:14

dlsniper marked this pull request as ready for review March 24, 2023 19:20

dlsniper requested review from AnomalRoil, CluEleSsUK, nikkolasg and willscott as code owners March 24, 2023 19:20

AnomalRoil reviewed Mar 30, 2023

View reviewed changes

dlsniper commented Mar 30, 2023

View reviewed changes

CluEleSsUK reviewed Mar 31, 2023

View reviewed changes

dlsniper added 8 commits April 12, 2023 12:03

Add OpenTelemetry tracing instrumentation

a312ef3

Initial commit for adding OT tracing using a customizable endpoint

Fixes after rebase

ce821c2

Misc cleanup

323103c

Instrument the DKG code

4684552

Fix missing refactoring

3a6dd54

Updates per code review comments

3d286a0

Updates per code review comments

088ba9e

Make the linter happy

885cef4

CluEleSsUK force-pushed the instrumentation branch from cda7cb8 to 885cef4 Compare April 12, 2023 13:25

added contexts to migrate command

65e8b31

CluEleSsUK force-pushed the instrumentation branch from 9d6dd16 to 65e8b31 Compare April 12, 2023 13:38

Merge branch 'develop' into instrumentation

a6f710e

CluEleSsUK approved these changes Apr 12, 2023

View reviewed changes

CluEleSsUK merged commit 71589cb into drand:develop Apr 12, 2023

AnomalRoil reviewed May 15, 2023

View reviewed changes

AnomalRoil mentioned this pull request May 15, 2023

Addressing comments for #1199 #1236

Merged

GlacierWalrus mentioned this pull request May 23, 2024

laith/dkg store #1357

Closed

GlacierWalrus mentioned this pull request Jun 24, 2024

feature/release validation #1367

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add OpenTelemetry tracing instrumentation #1199

Add OpenTelemetry tracing instrumentation #1199

dlsniper commented Mar 7, 2023 •

edited

Loading

dlsniper left a comment

CluEleSsUK left a comment

CluEleSsUK Mar 31, 2023

dlsniper Mar 31, 2023

AnomalRoil Apr 1, 2023

dlsniper commented Mar 31, 2023

AnomalRoil left a comment

AnomalRoil May 5, 2023

AnomalRoil May 5, 2023

AnomalRoil May 5, 2023

AnomalRoil May 8, 2023

AnomalRoil May 8, 2023

Add OpenTelemetry tracing instrumentation #1199

Add OpenTelemetry tracing instrumentation #1199

Conversation

dlsniper commented Mar 7, 2023 • edited Loading

dlsniper left a comment

Choose a reason for hiding this comment

CluEleSsUK left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dlsniper commented Mar 31, 2023

AnomalRoil left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dlsniper commented Mar 7, 2023 •

edited

Loading