Skip to content
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

Moving repo to fuel-telemetry #1

Open
wants to merge 69 commits into
base: main
Choose a base branch
from
Open

Moving repo to fuel-telemetry #1

wants to merge 69 commits into from

Conversation

alfiedotwtf
Copy link
Collaborator

This PR carries over all the rewrite commits from forc-telemetry, and changes the repo name to fuel-telemetry.

@alfiedotwtf alfiedotwtf self-assigned this Feb 12, 2025
///
/// This function does the following:
/// - Creates a new `TelemetryLayer` `tracing` `Layer`
/// - Sets it as the global default `tracing` `Subscriber`
Copy link
Member

Choose a reason for hiding this comment

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

This sounds like it would override the subscriber from forc-tracing since there can only be one global default. Have you tested using both of them together?

Maybe instead of telemetry_init, this should be built in to init_tracing_subscriber in forc-tracing? There could be an option like telemetry: bool that when true, initializes with the custom layer that has .with_span_events(FmtSpan::CLOSE) to capture duration of #[instrument]ed functions

That way, it would be as simple as adding the telemetry: true option to any tools that already use forc-tracing. What do you think?

pub fn telemetry_init() -> Result<()> {
static TELEMETRY_INIT: LazyLock<Result<(WorkerGuard, Span)>> = LazyLock::new(|| {
let guard = TelemetryLayer::new_global_default_with_watchers()?;
let main_span = span!(Level::ERROR, "main", telemetry = true);
Copy link
Member

Choose a reason for hiding this comment

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

Why is this level ERROR?

.with(stdout_layer);

// Set our subscriber as the default global subscriber
tracing::subscriber::set_global_default(subscriber).unwrap();
Copy link
Member

Choose a reason for hiding this comment

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

https://docs.rs/tracing/latest/tracing/index.html#in-executables

Warning: In general, libraries should not call set_global_default()! Doing so will cause conflicts when executables that depend on the library try to set the default later.

Copy link
Member

@kayagokalp kayagokalp Feb 20, 2025

Choose a reason for hiding this comment

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

Maybe we can wrap this one, and utilize type system to make downstream aware of this fact and make it a hard compile error if they try to use it in a library.

pub struct Binary;
pub struct Library;

pub trait CanSetGlobal {}
impl CanSetGlobal for Binary {}

pub fn set_global_subscriber<T>(
    _crate_type: T,
    subscriber: impl tracing::Subscriber + Send + Sync + 'static,
) where 
    T: CanSetGlobal,
{
    tracing::subscriber::set_global_default(subscriber).unwrap();
}

The downside of this is unfortunately there is no way to detect crate type in compile time.

rust-lang/rust#20267

So this would be used like:

// In binary - compiles:
set_global_subscriber(Binary, my_subscriber);

// In library - compile error because `Library` doesn't implement `CanSetGlobal`:
set_global_subscriber(Library, my_subscriber);

Comment on lines +58 to +60
// Time to enable telemetry from now on by setting telemetry=true on a `Span`
let enabled_span = span!(Level::INFO, "root", telemetry=true);
let _span_guard = enabled_span.enter();

Choose a reason for hiding this comment

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

can we reduce this to 1 line?

Copy link
Member

Choose a reason for hiding this comment

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

https://docs.rs/tracing/latest/tracing/struct.Span.html#method.entered can be used to ge this in single line

let _span_guard = span!(Level::INFO, "root", telemetry=true).entered();


Telemetry files on disk are stored Base64 encoded. To peek into them:

cat ~/.fuelup/tmp/*.telemetry.* | while read line; do echo "$line" | base64 -d; echo; done

Choose a reason for hiding this comment

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

nit: wrap in markdown script:

Suggested change
cat ~/.fuelup/tmp/*.telemetry.* | while read line; do echo "$line" | base64 -d; echo; done
```sh
cat ~/.fuelup/tmp/*.telemetry.* | while read line; do echo "$line" | base64 -d; echo; done

Comment on lines +166 to +167
for f in ~/.fuelup/tmp/*.telemetry.*; do mv "$f" "$f.old"; done
touch -t 202501010101 ~/.fuelup/tmp/*

Choose a reason for hiding this comment

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

nit: also wrap in markdown sh here too

Choose a reason for hiding this comment

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

could be useful to have a docker command (or nix-shell command?) - to view/query the telemetry locally collected.
This would also help with debugging, testing and validating local collected traces - before sending them over the wire.

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.

5 participants