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

Delete emitter implementations #21

Merged
merged 1 commit into from
Oct 21, 2022
Merged

Delete emitter implementations #21

merged 1 commit into from
Oct 21, 2022

Conversation

abhinav
Copy link
Collaborator

@abhinav abhinav commented 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

Base automatically changed from mapend-sliceend-fix to main October 21, 2022 17:17
abhinav added a commit that referenced this pull request Oct 21, 2022
…ed (#20)

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.
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
Copy link
Contributor

@r-hang r-hang left a comment

Choose a reason for hiding this comment

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

In your experiment for #11 what was the naming of the API you used to provide the event structs to the CFF?

@abhinav abhinav merged commit 73a40fc into main Oct 21, 2022
@abhinav abhinav deleted the delete-emitters branch October 21, 2022 17:34
@abhinav
Copy link
Collaborator Author

abhinav commented Oct 21, 2022

In your experiment for #11 what was the naming of the API you used to provide the event structs to the CFF?

It was named EventLogger but I'm not tied to the idea.
We can call it Emitter, or EventEmitter if we want.

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.

Delete or move Emitter implementations
2 participants