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

runtime/trace: stub more functions #4570

Open
wants to merge 1 commit into
base: dev
Choose a base branch
from

Conversation

joonas
Copy link

@joonas joonas commented Oct 31, 2024

I came across the following error while I was looking at using the opentelemetry libraries with TinyGo:

# go.opentelemetry.io/otel/sdk/trace
../../../../pkg/mod/go.opentelemetry.io/otel/[email protected]/trace/span.go:820:9: undefined: rt.IsEnabled
../../../../pkg/mod/go.opentelemetry.io/otel/[email protected]/trace/span.go:824:19: undefined: rt.NewTask

I thought it would be nice to have these functions stubbed out so that it would be possible to get further along with libraries that might try to make use of these runtime/trace functions, if for no other reason than to be able to use the go:linkname directive to do something else with them.

Copy link
Member

@aykevl aykevl left a comment

Choose a reason for hiding this comment

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

Can you run go fmt on src/runtime/trace/trace.go`? The CI complains that it is not correctly formatted (probably because of the missing newline at the end of the file).

Other than that and the comment below, LGTM.


func Logf(ctx context.Context, category, format string, args ...any) {}

func WithRegion(ctx context.Context, regionType string, fn func()) {}
Copy link
Member

Choose a reason for hiding this comment

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

I don't think this is correct. The documentation says:

WithRegion starts a region associated with its calling goroutine, runs fn, and then ends the region.

So even if we don't implement tracing, I think WithRegion should at least call fn.

Copy link
Author

Choose a reason for hiding this comment

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

Good call, I've adjusted the function to call the passed in fn, thanks!

@joonas joonas force-pushed the runtime/trace-stub-more-functions branch from 2ab77f1 to 1852997 Compare October 31, 2024 15:37
@joonas joonas force-pushed the runtime/trace-stub-more-functions branch from 1852997 to 82d45f2 Compare October 31, 2024 19:06
@joonas
Copy link
Author

joonas commented Oct 31, 2024

Decided to force push to see if the Windows test fails consistently, since it seems to have passed in prior push.

Force push seems to have done the trick 👍

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.

2 participants