-
Notifications
You must be signed in to change notification settings - Fork 73
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
Open
Copilot
wants to merge
7
commits into
platform/0.16
Choose a base branch
from
copilot/add-lifecycle-telemetry
base: platform/0.16
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from 4 commits
Commits
Show all changes
7 commits
Select commit
Hold shift + click to select a range
9a7d334
Initial plan
Copilot e0cd8ee
Add telemetry specification for RFC #0131 - Build Observability
Copilot 7dfb4c8
Add telemetry outputs and inputs to buildpack specification
Copilot cf88fca
Add telemetry output to image extension specification
Copilot a75f3f3
Use CNB_OTEL_LOG_PATH instead of hardcoded telemetry paths
Copilot 5ac3690
Add MAY write telemetry clause to detect and build phases
Copilot 17e6edc
Revert buildpack.md and image_extension.md changes
Copilot File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Uh oh!
There was an error while loading. Please reload this page.
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.jsonlfiles.However, from the line above the one added here there is:
...which would suggest that
$CNB_LAYERS_DIRis instead not/layersbut/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:
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_DIRwas the/layersin the context of both platform and buildpack - because it is/layerson the produced image.CNB_LAYERS_DIR=/layersis what is exported on the image and what is used forlifecycleitself during execution.It makes sense though, looking at buildpack spec, that
CNB_LAYERS_DIRis actually/layers/<buildpack-id>in the context of a buildpack execution.lifecyclemutates 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
/layersthough.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:
tracingdir in/layers/<buildpack-id>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.
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:
The drawback of this proposal is that we are polluting the
CNB_LAYERS_DIRwith 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
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 writejsonlCNB_OTEL_LOG_PATHand not rely on a static path in the 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.
How do the spec changes look @edmorley @jkutner ? I introduced the
CNB_OTEL_LOG_PATHThere 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