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

Fallback to the single offset if possible and version can't be parsed #503

Merged
merged 5 commits into from
Nov 16, 2023

Conversation

RonFed
Copy link
Contributor

@RonFed RonFed commented Nov 15, 2023

The main goal of this PR is to handle cases where an offset is required for a package with an un-official version (contains commit hash: for example v0.0.0-20230227094218-b8c73b2037b8
Currenly we can't handle such cases since we don't find a valid version.
I added a fallback to check if only a single offset were saved across all versions to use that offset. If more than one offset was saved and the version is not foundd, fail.
A more robust solution is probably required to handle this kind of version, but this fallback is a good solution until then.

@RonFed RonFed requested a review from a team as a code owner November 15, 2023 13:45
Copy link
Contributor

@MrAlias MrAlias left a comment

Choose a reason for hiding this comment

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

What are the consequences if the offset is wrong here? I'm guessing the best case scenario is the telemetry contains garbage, but is the worse case that the user application crashes?

I worry that this guess becomes problematic at the end of the version range. If a user is asking for an offset that is released after our last know version we have less confidence that value hasn't changed. Can we address that?

internal/pkg/inject/offset_results.json Outdated Show resolved Hide resolved
internal/pkg/structfield/structfield.go Outdated Show resolved Hide resolved
Copy link
Contributor

@MrAlias MrAlias left a comment

Choose a reason for hiding this comment

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

What are the consequences if the offset is wrong here? I'm guessing the best case scenario is the telemetry contains garbage, but is the worse case that the user application crashes?

I worry that this guess becomes problematic at the end of the version range. If a user is asking for an offset that is released after our last know version we have less confidence that value hasn't changed. Can we address that?

@RonFed
Copy link
Contributor Author

RonFed commented Nov 15, 2023

What are the consequences if the offset is wrong here? I'm guessing the best case scenario is the telemetry contains garbage, but is the worse case that the user application crashes?

I worry that this guess becomes problematic at the end of the version range. If a user is asking for an offset that is released after our last know version we have less confidence that value hasn't changed. Can we address that?

It depends if the offset is used to read from or write to the user app. Most (all except one that I can think of) of the offsets are used for attribute collection, so a wrong offset will lead to garbage value in the worst case (some probes drop the span if an attribute cna't be read, maybe we should change that). For those we use for writing (adding HTTP header for example), a wrong offset can cause a crash in the worst case. There are a lot of checks in the eBPF code in the writing to user memory parts, to minimize that chance, and the only case which I can think of this might happen is the HTTP header update (the offset there is part of the standard library)

Regarding newer versions, we can identify this case as a different one from the hash commit version and maybe act differently?

@edeNFed edeNFed merged commit 7bb1cdd into open-telemetry:main Nov 16, 2023
13 of 14 checks passed
@MrAlias MrAlias added this to the v0.9.0-alpha milestone Dec 12, 2023
@MrAlias MrAlias mentioned this pull request Dec 12, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants