Add JSON support for DefaultFields #3333
Open
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
The JSON formatter depends on the JsonFields FieldFormatter being in use. When .json() is called on a SubscriberBuilder directly this is handled automatically. However, when .json() is called on the Format given to .event_format() method of SubscriberBuilder a runtime error occurs unless the user also calls .json() on the SubscriberBuilder itself. This change correctly declares that the json code is only implemented for the JsonFields formatter. This results in compile-time errors, instead of a runtime error, unless the user calls .json() on the SubscriberBuilder *before* calling .event_format(). This improves the situation described in issue 3327, but a full resolution would involve either implementing the json code for the DefaultFields formatter as well (and by extension all future formatters added to the project), improving the error message so that the compiler recommends calling .json() on the SubscriberBuilder (it currently recommends removing the .json() call from the Format object), or making the SubscriberBuilder automatically use JsonFields when using the JSON output format.
For two of the methods the only constraint is that SerializableSpan has to implement Serialize for N/JsonFields. Make them generic again by adding this constraint, which will reduce code duplication when adding support for DefaultFields.
skyvine
changed the title
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Motivation
Currently, passing
format().json()to theevent_formatmethod ofSubscriberBuilderwithout also calling.json()on theSubscriberBuilderresults in either a runtime panic (in debug mode) or missing information from the log output (in release mode). See #3327 for more details including reproducer code and output.Solution
This change updates the current implementation to correctly declare that it only supports
JsonFieldsand adds a second implementation that supportsDefaultFields. The code in #3327 produces the expected output after this change.