Refactor exporter - step 2 #1081
Conversation
Codecov Report
@@ Coverage Diff @@
## master #1081 +/- ##
==========================================
- Coverage 77.35% 76.68% -0.68%
==========================================
Files 221 223 +2
Lines 6104 6158 +54
==========================================
Hits 4722 4722
- Misses 1382 1436 +54
|
| { | ||
| if (!this.stopped) | ||
| { | ||
| this.exporter.Shutdown(); |
There was a problem hiding this comment.
@reyang I merged to unblock you, but probably need try/catch in a future PR?
There was a problem hiding this comment.
I need to get feedback here.
The spec is saying that exporter should not throw.
I had similar struggle here, whether should I trust the other processors won't throw, or I should make the composite one bullet proof.
There was a problem hiding this comment.
So the higher order question is, how much do we trust / rely on the other components (whether processors/exporters that we own / in this repo, or we don't own / in other repos) to follow the spec / be a good citizen.
There was a problem hiding this comment.
Here is my take while I was a C/C++ runtime library dev:
- If a developer calls
freeon a buffer (allocated by malloc) that is already freed, we want to fail him instead of keep silent. In this way we are actually helping that developer to discover his bug, instead of hiding it since his bug could potentially has other implications (e.g. data loss, corruption, logical flaw). - If the heap manager observed any sign of an incorrect heap (e.g. via integrity check on sentry bytes), it should crash the app ASAP instead of trying to hide the mistake (since the app might run on dangling pointers which writes dirty data to a database/file).
However, here I'm struggling as I believe we have another design principle - a telemetry SDK, once initialized, should not punish the users by killing their applications.
There was a problem hiding this comment.
It is mainly from the responsibility and dev experience perspective (no perf concern here).
For example if some bad guy developed a broken exporter that throws (against the spec law) and we swallowed the exception for him, it seems like we are contributing to a crime instead of reporting to the cops.
And if a good guy is developing an exporter where he made subtle mistake, by throwing error directly we helped him to fix his bug. Otherwise he will have to look at the self diagnostic logs.
Anyways, I'm struggling here which means I don't have strong preference (and as you can see, I do have strong preference that we should catch Dispose exception and I put that catch statement 😄). So I'm happy to change either way (or even have a global flag to turn on/off such behavior) if @alanwest @CodeBlanch or anyone in the community has a strong preference.
There was a problem hiding this comment.
As usual, @reyang raises good points! I'm good with letting it throw. But I will say this... that doesn't necessarily guarantee anyone will pay attention! I have seen a lot of code get to prod where ops immediately starts complaining things won't shut down. Launch the app in dev, it is easily reproducable. How does this get by the devs? Because they use the "stop" button in VS which doesn't perform graceful shutdown 🙄
There was a problem hiding this comment.
Yes, definitely good points to ponder. I'm also good with letting it throw.
Though, I've generally thought of the telemetry SDK and other profiling tools as similar to logging frameworks where "do no harm" is often the general mantra. Usually (hopefully) there are other signs that something is awry - like a sudden drop of data reported to your backend, even though seemingly your app is still running "just fine".
This conversation is definitely bigger than just exporter or processors - these questions would apply to instrumentation among other things as well. It's more of general design philosophy, maybe more appropriate for a spec-level kind of guidance/adoption?
There was a problem hiding this comment.
A slightly different story from the Nt vs. Zw API.
I think given that none of us have strong opinion, and it seems @alanwest @CodeBlanch both have/had the intuition that it is nicer to swallow exception, we probably should go with the direction (to swallow exceptions) for now and make sure we handle this consistently across the repo?
There was a problem hiding this comment.
If I got two 👍 I'll send a PR to swallow the exception for both the exporting processor and composite processor.
This is a follow up PR of #1078.
Changes
SimpleExportActivityProcessor.BatchExportActivityProcessor.usingstatements.For significant contributions please make sure you have completed the following items: