Skip to content

Conversation

ndr-ds
Copy link
Contributor

@ndr-ds ndr-ds commented Sep 12, 2025

Motivation

Now that we have distributed tracing (after #4556), we need more instrumentation so we have data about more functions in the breakdowns.

Proposal

Instrument more functions with telemetry_only so that we don't get spammed in our logs, but the spans still get sent to Tempo.

Test Plan

Tested this with #4556, saw the spans properly show in the breakdowns.

Release Plan

  • Nothing to do / These changes follow the usual release cycle.

Copy link
Contributor Author

ndr-ds commented Sep 12, 2025

@ndr-ds ndr-ds changed the base branch from 09-10-use_release_max_level_trace_instead to graphite-base/4555 September 12, 2025 16:26
@ndr-ds ndr-ds force-pushed the 09-10-add_more_instrumentation branch from e3a8454 to 599673a Compare September 12, 2025 16:26
@ndr-ds ndr-ds changed the base branch from graphite-base/4555 to 09-09-adding_tempo_for_open_tracing September 12, 2025 16:26
@ndr-ds ndr-ds mentioned this pull request Sep 12, 2025
@ndr-ds ndr-ds changed the base branch from 09-09-adding_tempo_for_open_tracing to graphite-base/4555 September 12, 2025 19:29
@ndr-ds ndr-ds force-pushed the 09-10-add_more_instrumentation branch from 599673a to 45b67e5 Compare September 12, 2025 19:58
@ndr-ds ndr-ds changed the base branch from graphite-base/4555 to 09-09-adding_tempo_for_open_tracing September 12, 2025 19:58
@ndr-ds ndr-ds force-pushed the 09-10-add_more_instrumentation branch from 45b67e5 to 1736f76 Compare September 12, 2025 20:04
@ndr-ds ndr-ds force-pushed the 09-09-adding_tempo_for_open_tracing branch 2 times, most recently from 8125d96 to 0cc954c Compare September 12, 2025 20:11
@ndr-ds ndr-ds force-pushed the 09-10-add_more_instrumentation branch from 1736f76 to e700d67 Compare September 12, 2025 20:11
@ndr-ds ndr-ds force-pushed the 09-09-adding_tempo_for_open_tracing branch from 0cc954c to 76f7ddd Compare September 12, 2025 20:46
@ndr-ds ndr-ds force-pushed the 09-10-add_more_instrumentation branch 2 times, most recently from 7a6a2aa to 59b4fa0 Compare September 12, 2025 22:27
@ndr-ds ndr-ds force-pushed the 09-09-adding_tempo_for_open_tracing branch from 76f7ddd to 7131de0 Compare September 12, 2025 22:27
@ndr-ds ndr-ds force-pushed the 09-10-add_more_instrumentation branch from 59b4fa0 to e46dc1a Compare September 12, 2025 22:57
@ndr-ds ndr-ds force-pushed the 09-09-adding_tempo_for_open_tracing branch from 7131de0 to d6d672c Compare September 12, 2025 22:57
@ndr-ds ndr-ds force-pushed the 09-10-add_more_instrumentation branch from e46dc1a to f1302de Compare September 15, 2025 13:39
@ndr-ds ndr-ds force-pushed the 09-10-add_more_instrumentation branch from 3c906b7 to 3de7061 Compare September 18, 2025 01:28
@ndr-ds ndr-ds requested review from afck, deuszx, eldios, ma2bd and Twey September 18, 2025 01:50
@ndr-ds ndr-ds marked this pull request as ready for review September 18, 2025 01:50

#[instrument(target = "telemetry_only", skip_all, fields(
chain_id = %self.chain_id(),
next_block_height = %self.tip_state.get().next_block_height
Copy link
Contributor

Choose a reason for hiding this comment

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

I think the block height is unnecessary detail, it will obscure the trace.


#[instrument(target = "telemetry_only", skip_all, fields(
chain_id = %self.chain_id(),
target = %target,
Copy link
Contributor

Choose a reason for hiding this comment

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

We should really decide how we label things. I've seen things like nickname , target, etc. and the are really NOT helping.

/// Removes the incoming message bundles in the block from the inboxes.
#[instrument(target = "telemetry_only", skip_all, fields(
chain_id = %self.chain_id(),
timestamp = %timestamp
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd drop timestamp from there.


/// Runs the worker until there are no more incoming requests.
#[instrument(
target = "telemetry_only",
Copy link
Contributor

Choose a reason for hiding this comment

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

So we won't see this in the normal logs anymore?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will remove

break; // Request sender was dropped.
};
Box::pin(worker.handle_request(request).instrument(span)).await;
Box::pin(worker.handle_request(request)).instrument(span).await;
Copy link
Contributor

Choose a reason for hiding this comment

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

Small change but are you sure this is what we want? @Twey has more experience with these spans.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If we instrument inside the pin, we reach the compilation recursion limit


/// Handles a request and applies it to the chain state.
#[instrument(skip_all)]
#[instrument(target = "telemetry_only", skip_all, fields(chain_id = %self.chain_id()))]
Copy link
Contributor

Choose a reason for hiding this comment

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

Same – this won't show up in the normal logs anymore?

/// Returns the requested blob, if it belongs to the current locking block or pending proposal.
#[instrument(target = "telemetry_only", skip_all, fields(
chain_id = %self.chain_id(),
blob_id = %blob_id
Copy link
Contributor

Choose a reason for hiding this comment

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

blob_id is IMHO unnecessary.

Copy link
Contributor

@deuszx deuszx left a comment

Choose a reason for hiding this comment

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

I think we should discuss which logs we want for telemtry-only vs log file. Also, I think some details in the trace are unneccessary: timestamp, block height.

@ndr-ds ndr-ds force-pushed the 09-09-adding_tempo_for_open_tracing branch from f62b7d2 to cfa2f37 Compare September 22, 2025 13:50
@ndr-ds ndr-ds force-pushed the 09-10-add_more_instrumentation branch 2 times, most recently from 82bf2c1 to a866aa0 Compare September 22, 2025 14:17
Base automatically changed from 09-09-adding_tempo_for_open_tracing to main September 22, 2025 15:07
Copy link
Contributor

@deuszx deuszx left a comment

Choose a reason for hiding this comment

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

I think a lot of logging, especially in grpc.rs was read in the console/log file. Now it's all telemetry-only.

Copy link
Contributor Author

ndr-ds commented Sep 22, 2025

Ah, you're right! I'll do a pass, and if it was already INFO level before, the span should be sent already even without the telemetry_only thing. So I'll make sure that behavior is maintained for those before I merge

@ndr-ds ndr-ds force-pushed the 09-10-add_more_instrumentation branch from a866aa0 to b9e388c Compare September 22, 2025 17:12
@ndr-ds ndr-ds force-pushed the 09-10-add_more_instrumentation branch from b9e388c to 21cef69 Compare September 22, 2025 17:24
@ndr-ds ndr-ds added this pull request to the merge queue Sep 22, 2025
Merged via the queue into main with commit 0982bfd Sep 22, 2025
32 checks passed
@ndr-ds ndr-ds deleted the 09-10-add_more_instrumentation branch September 22, 2025 20:38
ndr-ds added a commit that referenced this pull request Sep 22, 2025
Now that we have distributed tracing (after
#4556), we need more
instrumentation so we have data about more functions in the breakdowns.

Instrument more functions with `telemetry_only` so that we don't get
spammed in our logs, but the spans still get sent to Tempo.

Tested this with #4556,
saw the spans properly show in the breakdowns.

- Nothing to do / These changes follow the usual release cycle.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants