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

[o11y] Add traceId to user trace #3209

Draft
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

fhanau
Copy link
Collaborator

@fhanau fhanau commented Dec 4, 2024

Builds on #3180. Also see the corresponding upstream PR.

@@ -92,6 +92,9 @@ class TraceId final {
// Replicates W3C Serialization
kj::String toW3C() const;

// Return network order hex representation
kj::String toNEHex() const;
Copy link
Member

Choose a reason for hiding this comment

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

Nit: While it's more verbose, I prefer avoiding the abbreviation here. s/toNEHex/toNetworkOrderHex

@@ -609,6 +616,7 @@ class WorkerTracer final: public kj::Refcounted, public BaseTracer {

void addLog(kj::Date timestamp, LogLevel logLevel, kj::String message) override;
void addSpan(CompleteSpan&& span) override;
void setTraceId(tracing::TraceId traceId);
Copy link
Member

@jasnell jasnell Dec 4, 2024

Choose a reason for hiding this comment

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

Nit: const tracing::TraceId& to prevent the extra copy? I know it's a small class but still. Could also get by with having the trace->traceId = traceId in the impl be a trace->traceId = kj::mv(traceId)

@fhanau fhanau force-pushed the felix/111524-user-tracing-otel-v1 branch 3 times, most recently from 598986f to 956a297 Compare December 18, 2024 16:51
Base automatically changed from felix/111524-user-tracing-otel-v1 to main December 18, 2024 17:39
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