-
Notifications
You must be signed in to change notification settings - Fork 72
Implement RFC #0131 - Build Observability telemetry specification (Platform spec) #421
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
base: platform/0.16
Are you sure you want to change the base?
Conversation
- Added Telemetry section describing lifecycle and buildpack telemetry files - Documented telemetry file locations under /layers/tracing/ - Specified OpenTelemetry File Exporter format in JSONL - Added context propagation via CNB_OTEL_TRACEPARENT environment variable - Documented telemetry file properties and consumption guidelines - Added Platform-Provided Variables section for CNB_OTEL_TRACEPARENT - Updated table of contents with new sections Co-authored-by: jabrown85 <790999+jabrown85@users.noreply.github.com>
- Added CNB_OTEL_TRACEPARENT input to detect and build phases - Added telemetry file outputs to detect and build phases - Documented optional telemetry file paths using OpenTelemetry File Exporter format Co-authored-by: jabrown85 <790999+jabrown85@users.noreply.github.com>
- Documented optional telemetry file output for extensions in detect and generate phases - Extensions can write telemetry to /layers/tracing/extensions/ directory Co-authored-by: jabrown85 <790999+jabrown85@users.noreply.github.com>
jabrown85
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks pretty good to me
buildpack.md
Outdated
| | `$CNB_LAYERS_DIR/<layer>/env.launch/<process>/` | Env vars for launch (after `env`, before `exec.d`) for the launched process | | ||
| | `$CNB_LAYERS_DIR/<layer>/env.build/` | Env vars for subsequent buildpacks (after `env`) | | ||
| | `$CNB_LAYERS_DIR/<layer>/*` | Other content for launch and/or subsequent buildpacks | | ||
| | `$CNB_LAYERS_DIR/tracing/buildpacks/<id>@<version>-build.jsonl` | Buildpack telemetry in [OpenTelemetry File Exporter format](https://opentelemetry.io/docs/specs/otel/protocol/file-exporter/) (optional) | |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm a bit confused by this.
From reading the RFC, I had thought the new tracing/ subdirectory was going at the very root of the layers dir hierarchy (ie /layers/tracing/...), hence the buildpack name and ID being included the the filenames of the .jsonl files.
However, from the line above the one added here there is:
|
$CNB_LAYERS_DIR/<layer>/*| Other content for launch and/or subsequent buildpacks
...which would suggest that $CNB_LAYERS_DIR is instead not /layers but /layers/<buildpack_id>, given that buildpack layers gets nested under a new directory for each buildpack? And if that were the case, then the resultant new tracing files location would resolve to /layers/<buildpack_id>/tracing/... which doesn't seem to match the spec?
Or does it just mean there is a typo in the original content of the spec here?
ie Should the existing line above instead say:
|
$CNB_LAYERS_DIR/<buildpack_id>/<layer>/*| Other content for launch and/or subsequent buildpacks
If there is an existing typo, then it's unrelated to this PR (but we should fix it at some point), but it would at least explain the difference here. However, if there isn't an existing typo, then I'm wondering if the new addition for this PR is correct?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's a good spot.
I looked earlier and thought CNB_LAYERS_DIR was the /layers in the context of both platform and buildpack - because it is /layers on the produced image. CNB_LAYERS_DIR=/layers is what is exported on the image and what is used for lifecycle itself during execution.
It makes sense though, looking at buildpack spec, that CNB_LAYERS_DIR is actually /layers/<buildpack-id> in the context of a buildpack execution. lifecycle mutates this in the execution context of each buildpack.
The spec isn't released yet so we have time to adjust this. I don't think buildpacks have access to anything that represents /layers though.
WDYT @hone @edmorley @joshwlewis?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yea, i think these are both true:
- We want the
tracingdir in/layers/<buildpack-id> - The
CNB_LAYERS_DIRis/layers/<buildpack-id>
If those are true, then I think we have it right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We want the
tracingdir in/layers/<buildpack-id>
The RFC says says this isn't the case though?
The structure has generic things at the top (eg lifecycle otel files), and then the buildpack files are named with the ID etc.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok, i see now. Here's one proposal to resolve this (but I also want to see an alternative from Jesse):
Location
Tracing files will be written to different locations depending on their source.
All lifecycle execution traces SHOULD be written to /layers/tracing/lifecycle/{phase}.jsonl.
Buildpack traces MAY be written to${CNB_LAYERS_DIR}/tracing-{phase}.jsonl.
Extension traces MAY be written to/layers/tracing/extensions/{id}@{version}-{phase}.jsonl.
A completed build with tracing might have a tracing file hierarchy like this:
<layers>
├── tracing
├── extensions
│ ├── some-id@some-version-detect.jsonl
│ └── some-id@some-version-generate.jsonl
└── lifecycle
├── analyze.jsonl
├── build.jsonl
├── detect.jsonl
├── export.jsonl
├── extend.jsonl
└── restore.jsonl
└── <buildpack-id>
├── tracing-build.jsonl
└── tracing-detect.jsonl
The drawback of this proposal is that we are polluting the CNB_LAYERS_DIR with non-layer information.
There was a problem hiding this 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 go with Jesse's proposal as it seems to be cleaner within lifecycle
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
WDYT @edmorley @joshwlewis?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@copilot update the spec with the following changes
- In the buildpack spec,
CNB_OTEL_LOG_PATHis passed to buildpacks in each phase for them to write to when present. Buildpacks will no longer useCNB_LAYERS_DIRto build their own path to writejsonl - The extension spec will also change to allow them to write to
CNB_OTEL_LOG_PATHand not rely on a static path in the spec. - In the platform spec,
CNB_OTEL_LOG_PATHshould be passed to buildpacks and point to a$CNB_LAYERS_DIR/tracing/buildpacks/<buildpack>@<version>-<phase>.jsonlpath created by the platform. Similar changes to extensions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm
|
@copilot In the buildpack spec, CNB_OTEL_LOG_PATH is passed to buildpacks in each phase for them to write to when present. Buildpacks will no longer use CNB_LAYERS_DIR to build their own path to write jsonl |
- Add CNB_OTEL_LOG_PATH as input to buildpack detect and build phases - Add CNB_OTEL_LOG_PATH as input for image extensions - Replace hardcoded telemetry output paths with CNB_OTEL_LOG_PATH - Document CNB_OTEL_LOG_PATH in platform spec as platform-provided variable - Update telemetry section to describe platform provides path to buildpacks/extensions Co-authored-by: jabrown85 <790999+jabrown85@users.noreply.github.com>
Updated in commit a75f3f3. Changes made:
|
|
@copilot The buildpack spec needs a similar "MAY write telemetry data to |
- Added "MAY write telemetry data to CNB_OTEL_LOG_PATH" to detect phase - Added "MAY write telemetry data to CNB_OTEL_LOG_PATH" to build phase - Matches the format used in image extension spec Co-authored-by: jabrown85 <790999+jabrown85@users.noreply.github.com>
Added in commit 5ac3690. Changes made:
|
|
@jabrown85 can we split this for buildpack + platform so we can merge this into the correct branches? |
|
@copilot split this PR into 2 distinct PRs. This PR will contain
|
This PR now only contains platform.md changes and will target platform/0.16. buildpack.md and image_extension.md changes will be moved to a separate PR targeting buildpack/0.13. Co-authored-by: jabrown85 <790999+jabrown85@users.noreply.github.com>
Split completed in commit 17e6edc. This PR now contains only For the buildpack changes, a new PR will need to be created with the following changes targeting
The reverted commits that contain these buildpack changes are: 7dfb4c8, cf88fca, a75f3f3 (partial), and 5ac3690. |
Adds telemetry specification for the platform to support lifecycle telemetry as described in RFC #0131.
Note: This PR contains only platform specification changes and targets
platform/0.16. Buildpack and image extension specification changes will be submitted in a separate PR targetingbuildpack/0.13.Changes
platform.md
/layers/tracing/directory structure for lifecycle, buildpack, and extension telemetry files using OpenTelemetry File Exporter format (JSONL)CNB_OTEL_TRACEPARENTenvironment variable for W3C Trace Context correlationCNB_OTEL_LOG_PATHenvironment variable that platform provides to buildpacks and extensions, pointing to<layers>/tracing/buildpacks/or<layers>/tracing/extensions/Telemetry file hierarchy
All telemetry emission is optional. Platform controls telemetry collection by providing
CNB_OTEL_LOG_PATHto buildpacks and extensions. Platform reads files during or after build for observability purposes.Original prompt
💬 We'd love your input! Share your thoughts on Copilot coding agent in our 2 minute survey.