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

fix({Slice,Map}End): Not allowed with ContinueOnError when instrumented #20

Merged
merged 1 commit into from
Oct 21, 2022

Conversation

abhinav
Copy link
Collaborator

@abhinav abhinav commented Oct 21, 2022

The compile-time check that verifies that SliceEnd and MapEnd are not
allowed with ContinueOnError
was part of the function that validates instrumentation.

This introduced a bug: if that function returned early
because the number of emitters was non-zero,
it would never run this check.

Fix this and add a failing test case.

This allowed a bug in our magic.go example files,
because they were using SliceEnd/MapEnd despite ContinueOnError.

Found while working on #21.

The compile-time check that verifies that SliceEnd and MapEnd are not
allowed with ContinueOnError
was part of the function that validates instrumentation.

This introduced a bug: if that function returned early
because the number of emitters was non-zero,
it would never run this check.

Fix this and add a failing test case.

This allowed a bug in our magic.go example files,
because they were using SliceEnd/MapEnd despite ContinueOnError.
@abhinav abhinav enabled auto-merge (squash) October 21, 2022 16:19
@abhinav abhinav disabled auto-merge October 21, 2022 16:21
@abhinav abhinav enabled auto-merge (squash) October 21, 2022 16:21
@abhinav abhinav merged commit 5525184 into main Oct 21, 2022
@abhinav abhinav deleted the mapend-sliceend-fix branch October 21, 2022 17:17
abhinav added a commit that referenced this pull request Oct 21, 2022
This deletes the Zap and Tally emitter implementations from cff to
reduce the dependency footprint.
We'll keep copies of these for use internally.

This also warns users away from using WithEmitter because the API is
expected to change significantly with #11.

Resolves #2
Depends on #20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants