feat(tracing): Move all performance monitoring code behind a compile flag #645
Conversation
CMakeLists.txt
Outdated
| @@ -559,6 +559,9 @@ if(SENTRY_BUILD_EXAMPLES) | |||
| add_executable(sentry_example examples/example.c) | |||
| target_link_libraries(sentry_example PRIVATE sentry) | |||
|
|
|||
| target_compile_definitions(sentry PRIVATE SENTRY_PERFORMANCE_MONITORING) | |||
There was a problem hiding this comment.
SENTRY_BUILD_EXAMPLES is I think the default if you directly compile the project (in contrast to compiling it as a sub-project), so IMO this goes a bit too far (enables it by default).
Better to make it a really opt-in flag, similar to the client-side stack trace feature.
There was a problem hiding this comment.
i don't think i have the knowledge to convert SENTRY_PERFORMANCE_MONITORING into a proper opt-in flag considering what i'm trying to prioritize right now:
- anybody building the example via
make buildshould get perf monitoring out of the box, targeted towards other devs who may be contributing to perfmon - unit tests should have perf monitoring enabled by default
i could create a separate make command to build the project with the performance monitoring flag enabled. this just raises other build issues with the unit tests though, as they're missing symbols locked behind the feature flag.
i'm open to any help to actually make this work, but right now automatically turning the flag on when SENTRY_BUILD_EXAMPLES is on appears to be my best bet to getting this working. the long term plan is to either remove the flag entirely or to fully commit to it anyways, so this should be a good enough temporary solution i think.
There was a problem hiding this comment.
i'll leave the PR open until the start of the work day tomorrow just to get some follow-up feedback on this and to see if there are any deal breakers in the current diff. otherwise, i'll be merging this in.
| @@ -153,3 +153,7 @@ if(SENTRY_INTEGRATION_QT) | |||
| integrations/sentry_integration_qt.h | |||
| ) | |||
| endif() | |||
|
|
|||
| if(SENTRY_BUILD_EXAMPLES) | |||
| target_compile_definitions(sentry PRIVATE SENTRY_PERFORMANCE_MONITORING) | |||
There was a problem hiding this comment.
this is redundant with the definition in the main cmake file.
| @@ -200,15 +200,23 @@ sentry_envelope_get_event(const sentry_envelope_t *envelope) | |||
| return sentry_value_new_null(); | |||
| } | |||
| for (size_t i = 0; i < envelope->contents.items.item_count; i++) { | |||
|
|
|||
| #ifdef SENTRY_PERFORMANCE_MONITORING | |||
There was a problem hiding this comment.
now that I see this again, maybe we should return the transaction here. remember: a transaction is an event, just a very special one. It has an id, and can have associated attachments.
There was a problem hiding this comment.
i'm going to avoid referencing more than one SDK here, but python for example deliberately has separate getters for an event and a transaction in an envelope, for a counterexample.
This moves both the external and the internal parts of the API related to performance monitoring (/tracing) behind a compile flag. This is to make it easier for us to iterate on the API, and to potentially break it or leave it in a compilable but incomplete state.
If an end user runs into any issues with the feature they must have deliberately opted into the feature and/or attempted to use something that isn't publicly documented and is therefore not yet publicly supported by the SDK yet.
relates to #601