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

[otep] Propose adding env variables as context carriers to specification #258

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

Conversation

adrielp
Copy link

@adrielp adrielp commented May 25, 2024

Based on conversations last week in the Specification and Semantic Conventions SIGs, I'm opening this duplicate pull request which was originally set as a Draft and hasn't had movement since last November.

There are real use cases that are coming to fruiting, namely in the CI/CD working group, that will benefit from this being accepted. Once accepted we can work on getting the specification added for both general context propagation and baggage.

On the note of baggage; baggage is a form of context propagation and was not originally mentioned directly by name in this OTEP. It is however, absolutely essential. I've had the pleasure of prototyping out tracing within an OpenTofu controller system where context on available in parent/child at the very start of the trace was available. Baggage was the means of transferring this critical context to subsequent siblings that would've not had it otherwise.

Thanks for all the hard work to the original author (@deejgregor) and opening the draft #241

CC. TC sponsors @jsuereth @carlosalberto

@adrielp adrielp requested a review from a team May 25, 2024 23:06
@deejgregor
Copy link

deejgregor commented May 25, 2024

@adrielp thank you for picking up this work. I appreciate that while I’ve been going through a busy time (and not sure when it will wrap up).

@adrielp adrielp changed the title [otep] add env-context-baggage-carriers otep [otep] Propose adding env variables as context carriers to specification May 26, 2024
text/0258-env-context-baggage-carriers.md Outdated Show resolved Hide resolved
text/0258-env-context-baggage-carriers.md Outdated Show resolved Hide resolved
text/0258-env-context-baggage-carriers.md Show resolved Hide resolved
text/0258-env-context-baggage-carriers.md Show resolved Hide resolved
text/0258-env-context-baggage-carriers.md Outdated Show resolved Hide resolved
text/0258-env-context-baggage-carriers.md Outdated Show resolved Hide resolved
Copy link
Member

@pellared pellared left a comment

Choose a reason for hiding this comment

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

👍

Copy link
Member

@pellared pellared left a comment

Choose a reason for hiding this comment

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

Awesome OTEP 👍

Copy link

@lmolkova lmolkova left a comment

Choose a reason for hiding this comment

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

Added some minor comments.

Approving under the assumption that traceparent and tracestate format is intended to be identical to the W3C Trace-Context spec (and that we document it explicitly)

> Text Map Propagation with a potential set of well-known environment variable
> names.

`traceparent` (lowercase), originates in the [W3C Specification][w3c-parent]
Copy link

Choose a reason for hiding this comment

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

nit:

Suggested change
`traceparent` (lowercase), originates in the [W3C Specification][w3c-parent]
`traceparent` (lowercase), originates in the [W3C Trace-Context][w3c-parent]

> Text Map Propagation with a potential set of well-known environment variable
> names.

`traceparent` (lowercase), originates in the [W3C Specification][w3c-parent]
Copy link

Choose a reason for hiding this comment

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

could we be more specific than "originates"? Is it exactly the same format? If so, could we call it out?

export TRACEPARENT=version=2HEXDIGLC,trace-id=32HEXDIGLC,parent-id=16HEXDIGLC,trace-flags=2HEXDIGLC
```

`tracestate` (lowercase), originates in the [W3C Specification][w2c-state] and
Copy link

Choose a reason for hiding this comment

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

Suggested change
`tracestate` (lowercase), originates in the [W3C Specification][w2c-state] and
`tracestate` (lowercase), originates in the [W3C Trace-Context][w3c-state] and

export TRACEPARENT=version=2HEXDIGLC,trace-id=32HEXDIGLC,parent-id=16HEXDIGLC,trace-flags=2HEXDIGLC
```

`tracestate` (lowercase), originates in the [W3C Specification][w2c-state] and
Copy link

@lmolkova lmolkova Oct 1, 2024

Choose a reason for hiding this comment

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

same note on "originates" - could we call out that the format is the same?

can include any opaque value in a key-value pair structure. Its goal is to
provide additional vendor-specific trace information.

`baggage` (lowercase), also is defined in the [W3C Specification][w3c-bag] and
Copy link

Choose a reason for hiding this comment

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

Suggested change
`baggage` (lowercase), also is defined in the [W3C Specification][w3c-bag] and
`baggage` (lowercase), also is defined in the [W3C Baggage][w3c-bag] and

spans. One example of this is module version and repository information. This
information is only determined and known during the `init` process. Subsequent
processes only know about the module by name. With `BAGGAGE` the rest of the
proccesses are able to understand a key piece of information which allows

Choose a reason for hiding this comment

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

Suggested change
proccesses are able to understand a key piece of information which allows
processes are able to understand a key piece of information which allows

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.

9 participants