Support template paths as part of span names for go versions >= 1.22

[RonFed]

In go v1.22 the net/http package added support for pattern handlers https://go.dev/blog/routing-enhancements.
This allows us to add a low cardinality path for span names in http server.
There were a few discussions about this topic (one of them in #627), and I think this solution allows for a much more informative span name while keeping it with low cardinality.

• Change the span name to {method} {http.route} when a route is available.
• Added the http.route attribute when the path template is available.
• Update the net/http e2e test to use a path template.

[damemi]

this LGTM, just want to double check this line from semconv:

Instrumentation MUST NOT default to using URI path as span name, but MAY provide hooks to allow custom logic to override the default span name.

I think what this is doing would count as "providing hooks" (using the option consts) rather than defaulting to using the path. But, our instrumentation agent will be using that hook by default. @MrAlias does this seem right or does that line mean this should be behind some non-default option for the agent?

[MrAlias]

this LGTM, just want to double check this line from semconv:

Instrumentation MUST NOT default to using URI path as span name, but MAY provide hooks to allow custom logic to override the default span name.

I think what this is doing would count as "providing hooks" (using the option consts) rather than defaulting to using the path. But, our instrumentation agent will be using that hook by default. @MrAlias does this seem right or does that line mean this should be behind some non-default option for the agent?

This seems correct to me given it will only ever use a templatized path. The hooks are there when a templitized version of this is not available in the language, but is something the user could provide.

[MrAlias]

For those also interested to see how this handles non-exact matches: https://go.dev/play/p/9ZZz4BjTQ6C

[damemi]

this LGTM, just want to double check this line from semconv:

Instrumentation MUST NOT default to using URI path as span name, but MAY provide hooks to allow custom logic to override the default span name.

I think what this is doing would count as "providing hooks" (using the option consts) rather than defaulting to using the path. But, our instrumentation agent will be using that hook by default. @MrAlias does this seem right or does that line mean this should be behind some non-default option for the agent?

This seems correct to me given it will only ever use a templatized path. The hooks are there when a templitized version of this is not available in the language, but is something the user could provide.

Ah so URI Path in that section of semconv is referring to the full non-templatized path. Makes sense thanks 👍

[RonFed]

@MrAlias Can you please have another look?

[MrAlias]

@MrAlias Can you please have another look?

We talked about this PR in the SIG meeting today. I'm hesitant to merge this without the offsets generator being fixed.

If I understand this correctly, if a user is using a version of Go prior to 1.22 the offsets will be "found" and the instrumentation loaded, but that offset is invalid an the system would fail to handle it.

I would not want to include this in the instrumentation without first having correct offsets.

[RonFed]

@MrAlias Can you please have another look?

We talked about this PR in the SIG meeting today. I'm hesitant to merge this without the offsets generator being fixed.

If I understand this correctly, if a user is using a version of Go prior to 1.22 the offsets will be "found" and the instrumentation loaded, but that offset is invalid an the system would fail to handle it.

I would not want to include this in the instrumentation without first having correct offsets.

The following part should make sure that if we instrument an app with go version lower than 1.22 the pattern parts are not taken into account (we won't consider the offsets in eBPF if we are not detecting a valid version), hence I think it should not necessarily block this PR.

type patternPathSupportedConst struct{}

var (
	patternPathMinVersion  = version.Must(version.NewVersion("1.22.0"))
	isPatternPathSupported = false
)

func (c patternPathSupportedConst) InjectOption(td *process.TargetDetails) (inject.Option, error) {
	isPatternPathSupported = td.GoVersion.GreaterThanOrEqual(patternPathMinVersion)
	return inject.WithKeyValue("pattern_path_supported", isPatternPathSupported), nil
}

[MrAlias]

Ah! I couldn't find this. Guess closing the other PR closed this. Thanks for reopening. I'll try to review this next.

[RonFed]

Not sure why the offsets action is failing with "msg"="failed get offsets" "error"="Error response from daemon: No such image: golang:1.14.1"

[MrAlias]

Not sure why the offsets action is failing with "msg"="failed get offsets" "error"="Error response from daemon: No such image: golang:1.14.1"

That is strange. The image does appear to still exist: https://hub.docker.com/layers/library/golang/1.14.1/images/sha256-23405c683ba88dffcb1493c66305533bb5b3cef179e419e2e5c2f5cb92a06891?context=explore

[MrAlias]

This seems unique to this PR. The auto-offset-gen action still works fine: https://github.com/open-telemetry/opentelemetry-go-instrumentation/actions/runs/8850141574/job/24303716999

[MrAlias]

I tried reproducing the failure locally, but was unsuccessful. I even deleted the local golang:1.14.1 image but it still succeeded. This looks unique to the action.

I'm wondering if there is some caching we can clear.

[MrAlias]

I'm able to reproduce in my fork: https://github.com/MrAlias/opentelemetry-go-instrumentation/actions/runs/8851008583/job/24306583275?pr=237

[MrAlias]

Yikes. Looks like there is no space left on the device(?)

2024/04/26 16:11:56 inspector/builder: "level"=1 "msg"="pulling image" "image"="golang:1.14.1" "output"="{"status":"Pulling from library/golang","id":"1.14.1"}\r\n{"status":"Already exists","progressDetail":{},"id":"f15005b0235f"}\r\n{"status":"Already exists","progressDetail":{},"id":"41ebfd3d2fd0"}\r\n{"status":"Already exists","progressDetail":{},"id":"b998346ba308"}\r\n{"status":"Already exists","progressDetail":{},"id":"f01ec562c947"}\r\n{"status":"Already exists","progressDetail":{},"id":"c5e092b8639b"}\r\n{"status":"Pulling fs layer","progressDetail":{},"id":"6ee82bba3116"}\r\n{"status":"Pulling fs layer","progressDetail":{},"id":"3b36b05a8864"}\r\n{"status":"Downloading","progressDetail":{"current":126,"total":126},"progress":"[==================================================\u003e] 126B/126B","id":"3b36b05a8864"}\r\n{"status":"Verifying Checksum","progressDetail":{},"id":"3b36b05a8864"}\r\n{"status":"Download complete","progressDetail":{},"id":"3b36b05a8864"}\r\n{"status":"Downloading","progressDetail":{"current":531698,"total":123632532},"progress":"[\u003e ] 531.7kB/123.6MB","id":"6ee82bba3116"}\r\n{"status":"Downloading","progressDetail":{"current":10180900,"total":123632532},"progress":"[====\u003e ] 10.18MB/123.6MB","id":"6ee82bba3116"}\r\n{"status":"Downloading","progressDetail":{"current":123632532,"total":123632532},"progress":"[==================================================\u003e] 123.6MB/123.6MB","id":"6ee82bba3116"}\r\n{"errorDetail":{"message":"write /var/lib/docker/tmp/GetImageBlob3791904830: no space left on device"},"error":"write /var/lib/docker/tmp/GetImageBlob3791904830: no space left on device"}\r\n"

https://github.com/MrAlias/opentelemetry-go-instrumentation/actions/runs/8851119331/job/24306956117?pr=237

I bumped the logging verbosity to show this.

[MrAlias]

Guessing this comes down to the fact that we are having to check all the old versions of Go for the new symbols added here. Prior to this we used to just hit the cache for all the symbols and never pulled the Go images.

[MrAlias]

Guessing this comes down to the fact that we are having to check all the old versions of Go for the new symbols added here. Prior to this we used to just hit the cache for all the symbols and never pulled the Go images.

Guessing we're going to need to add a "hint" to the offset manifests about what minimum version the symbol was added in.

[MrAlias]

Guessing this comes down to the fact that we are having to check all the old versions of Go for the new symbols added here. Prior to this we used to just hit the cache for all the symbols and never pulled the Go images.

Guessing we're going to need to add a "hint" to the offset manifests about what minimum version the symbol was added in.

#799

Going to try and resolve this later today.

[MrAlias]

@RonFed RonFed#1

That looks to resolve the "out of space" issue: https://github.com/MrAlias/opentelemetry-go-instrumentation/actions/runs/8852585249/job/24311585805?pr=237

[RonFed]

Guessing this comes down to the fact that we are having to check all the old versions of Go for the new symbols added here. Prior to this we used to just hit the cache for all the symbols and never pulled the Go images.

Yeah, that makes sense.

@RonFed RonFed#1

That looks to resolve the "out of space" issue: https://github.com/MrAlias/opentelemetry-go-instrumentation/actions/runs/8852585249/job/24311585805?pr=237

Do you think we should use this as a quick fix or add some generic logic to the cache lookup?

For general solution I think that if for a given module the minimal version is vModMin and some field in that module has cache entries starting from version vFieldMin and vFieldMin > vModMin we won't consider the versions smaller than vFieldMin

[MrAlias]

For general solution I think that if for a given module the minimal version is vModMin and some field in that module has cache entries starting from version vFieldMin and vFieldMin > vModMin we won't consider the versions smaller than vFieldMin

This would mean that patch releases to prior minor/major versions would not be evaluated. I don't think that is the correct approach.

[RonFed]

For general solution I think that if for a given module the minimal version is vModMin and some field in that module has cache entries starting from version vFieldMin and vFieldMin > vModMin we won't consider the versions smaller than vFieldMin

This would mean that patch releases to prior minor/major versions would not be evaluated. I don't think that is the correct approach.

Right. Another idea is to add "null" offsets to the cache for versions where there is no valid offset. It makes sense to cache the evaluation of no valid offset in a given version, but it will make the offset file much bigger.

[MrAlias]

Right. Another idea is to add "null" offsets to the cache for versions where there is no valid offset. It makes sense to cache the evaluation of no valid offset in a given version, but it will make the offset file much bigger.

That sounds reasonable to me. It would be a bit harder for a human to read, but that's not critical if we can solve #26