NH-42458 set_transaction_name empty name case #162
Conversation
There was a problem hiding this comment.
Thanks @tammy-baylis-swi, and really appreciate the enhanced docstring :). I left my opinion on the log levels--in general we try to use error level very sparingly, only for cases where library functionality is impacted/unrecoverable.
solarwinds_apm/api/__init__.py
Outdated
| if isinstance(get_tracer_provider(), NoOpTracerProvider): | ||
| logger.debug( | ||
| logger.warning( |
There was a problem hiding this comment.
Since there would already be a log message on startup if the agent is disabled, and it could actually be an intentional disabling, I think this message at the debug level makes more sense--esp. since in this scenario there would never be any trace data (thus transaction names) exported anyway.
There was a problem hiding this comment.
Good call! Don't need too much notification. Addressed in 32bf45e
solarwinds_apm/api/__init__.py
Outdated
| @@ -68,7 +75,7 @@ def set_transaction_name(custom_name: str) -> bool: | |||
| entry_trace_id = baggage.get_baggage(INTL_SWO_CURRENT_TRACE_ID) | |||
| entry_span_id = baggage.get_baggage(INTL_SWO_CURRENT_SPAN_ID) | |||
| if not entry_trace_id or not entry_span_id: | |||
| logger.debug( | |||
| logger.error( | |||
There was a problem hiding this comment.
I think warning might be more appropriate, similar to if the customer passed in an invalid transaction name.
There was a problem hiding this comment.
Thanks, make sense! Addressed in 32bf45e
|
Thank you @cheempz ! Please have another look when you have a moment. |
If customers call
set_transaction_name("")with an empty string, there will now be a warning log and the method will returnFalse. I've commented on the hijacked spec doc to confirm, and Java library is doing this too: https://github.com/appoptics/solarwinds-apm-java/pull/105/filesBefore this change, Python relies on falsiness to not use empty string nor None as transaction name: https://github.com/solarwindscloud/solarwinds-apm-python/blob/main/solarwinds_apm/inbound_metrics_processor.py#L171 So what's exported to SWO doesn't change with this PR.
I've also adjusted log severity of other parts of the method, though I'm not sure what's best with the return of
TruevsFalseand if it's a simple "ignore" or pretty-bad-can't-find-InboundMetricsSpanProcessor.Any suggestions welcome 😺