Add support for using rust-python-jaeger-reporter #7697
Conversation
| def report_span(self, span): | ||
| try: | ||
| return self._reporter.report_span(span) | ||
| except Exception: | ||
| logger.exception("Failed to report span") |
There was a problem hiding this comment.
Is this due to lack of confidence in the underlying report_span code or a just-in-case or?
Should this be in the RustReporter-class instead?
(Also, wouldn't this be simpler as a sub-class instead of a wrapper?)
There was a problem hiding this comment.
This is called from a lot of places, so an exception here can turn fairly fatal to the entire app, so I think an abundance of caution is probably warranted here.
We have basically three options here:
- Make the native class not raise, which is probably mostly doable, though a) there is a layer between the python and the native code that can raise exceptions and b) means figuring out logging from the rust library (which is probably fine).
- Move this python class into the library so that it becomes a mixed python/native library. I thought this would turn out hard, but actually looks like the tooling now supports it (some of their docs say it doesn't, but I think they're outdated)
- What we've done here.
We should probably do a combination of 1 and 2 TBH.
It's probably also worth noting that the rust library currently does throw occasionally since a) it assumes something is around to catch and log exceptions and b) the python jaeger client is really inconsistent with what data types pass to the Reporter objects, and so there is a bunch of code in the rust lib to change types from the python custom class to the thrift defined classes, and they're not totally fool proof. (Plus, if you try and give a native library an integer bigger than 128 bits things explode).
(Also, wouldn't this be simpler as a sub-class instead of a wrapper?)
Maybe? I'm not a super fan of sub classing and I'm not 100% if there are any gotchas with subclassing native types.
There was a problem hiding this comment.
We should probably do a combination of 1 and 2 TBH.
We could make this improvement in the future, I suppose. 👍
There was a problem hiding this comment.
Is there anything we should log here to help identify what happened to cause the exception? Or is using logger.exception enough to provide what we need?
There was a problem hiding this comment.
Logging the exception should tell us everything bar what span is, and sentry should tell us that.
|
In general I think that this is sane, by the way, it sounds like the worst that will happen is that reporting will break? Is the next step to merge this or to attempt to deploy this somewhere or what? |
That, and if there is a bug in the native library its possible some things get reporter wrong I guess.
It's currently on jki.re, so probably next steps are to merge and try on matrix.org. All the bugs that have come out of deploying on jki.re needed to be fixed in the rust lib rather than in python |
* commit 'e07a8caf5': Add support for using rust-python-jaeger-reporter (#7697)
This uses https://github.com/erikjohnston/rust-jaeger-python-client (which is published as a wheel on pypi). I'm not entirely sure we want to do this, but it does provide better performance than the pure python reporter.
FWIW the python version uses auto generated code via thrift to implement their reporter, the rust version does exactly the same and uses the auto generated thrift code.