Implementing ExtendedLogRecordData #1918
Conversation
| import io.opentelemetry.sdk.resources.Resource; | ||
| import javax.annotation.Nullable; | ||
|
|
||
| @SuppressWarnings({"deprecation", "SuppressWarningsWithoutExplanation"}) |
There was a problem hiding this comment.
Instead of SuppressWarningsWithoutExplanation we usually add the explanation. If possible move the deprecation closer to where the deprecated method is used.
Looking at the sdk code there are 2 implementations for these classes. The extended one when the incubator is available and the non-extended one that is used otherwise. Did you consider using the same pattern?
There was a problem hiding this comment.
Instead of SuppressWarningsWithoutExplanation we usually add the explanation. If possible move the deprecation closer to where the deprecated method is used.
Got it. I've just updated it.
Looking at the sdk code there are 2 implementations for these classes. The extended one when the incubator is available and the non-extended one that is used otherwise. Did you consider using the same pattern?
I think that's the approach we should take when/if we add early support for extended attributes in this module. I'm not sure for how long this feature is meant to stay in an incubating state, which is something I wanted to check in the issue thread but so far I haven't gotten feedback on that, so that if it's not too long, maybe we won't need to create 2 impls if we can just wait for the stable interface to get extended. For now, my goal with these changes is to at least keep the "old/stable impl" working in the meantime, since all logs are broken atm.
laurit
left a comment
laurit
left a comment
There was a problem hiding this comment.
I think it is fine for a temporary workaround.
4 participants
These changes are meant as a temporary workaround for this issue.
We need to decide whether we are going to add support for extended attributes in disk buffering while they are incubating. However, in the meantime these changes will prevent the issue mentioned above where logs are completely broken due to the cast issue.