Repository navigation
feat(options): add option to disable the Server-Timing header - #921
arjun-rajappa wants to merge 1 commit into
Conversation
Signed-off-by: Arjun Rajappa <arjun.rajappa@ibm.com>
CagriYonca
left a comment
CagriYonca
left a comment
There was a problem hiding this comment.
I left a couple of comments. What do you think?
| from instana.singletons import env_is_aws_lambda, get_agent, get_tracer | ||
| from instana.util.ids import define_server_timing | ||
|
|
||
| def add_server_timing_header(result: Dict[str, Any], trace_id: int) -> None: |
There was a problem hiding this comment.
Could we use dict instead of Dict? And avoid using Any if possible
There was a problem hiding this comment.
Agree with @CagriYonca.
Since Python 3.9, we can use the built-ins dict, list, and tuple instead of the imported Dict, List, and Tuple from the typing module.
I don't like the indiscriminate use of Any, but Bob does. If you know the possible types or classes for the dictionary values, please list them.
| get_disable_trace_configurations_from_env, | ||
| get_disable_trace_configurations_from_local, | ||
| get_disable_trace_configurations_from_yaml, | ||
| get_disable_server_timing_from_yaml, |
There was a problem hiding this comment.
Move this above get_disable_trace_configurations_* to keep the list alphabetical.
| Apply tracing.global.disable-server-timing from agent config (lowest | ||
| priority: the environment variable and the YAML file take precedence). | ||
| """ | ||
| global_config = tracing.get("global") |
There was a problem hiding this comment.
Could we add this parameter to the configuration test yaml files?
| span_id = format_span_id(span_context.span_id).encode() | ||
| level = str(span_context.level).encode() | ||
| server_timing = define_server_timing(span_context.trace_id).encode() | ||
| server_timing = ( |
There was a problem hiding this comment.
If server_timing is not used anywhere else, can we change this and move it to line 67?
if not self.is_server_timing_disabled():
headers.append((self.HEADER_SERVER_TIMING, define_server_timing(span_context.trace_id).encode()))There was a problem hiding this comment.
I do prefer the way @arjun-rajappa proposed here. With that, we have a common place to set all necessary values, even if used just once.
| trace_id = format_span_id(span_context.trace_id) | ||
| span_id = format_span_id(span_context.span_id) | ||
| server_timing = define_server_timing(span_context.trace_id).encode() | ||
| server_timing = ( |
|
|
||
| def test_exception_defaults_to_enabled(self) -> None: | ||
| with patch("instana.singletons.get_agent", side_effect=RuntimeError("boom")): | ||
| assert BasePropagator.is_server_timing_disabled() is False |
There was a problem hiding this comment.
Can we check whether the debug log is emitted by caplog or not?
| synthetic=False, | ||
| ) | ||
|
|
||
| def test_dict(self, trace_id: int, span_id: int, hex_trace_id: str) -> None: |
There was a problem hiding this comment.
Could we use self-explainer test function names, such as test_binary_inject_dict_server_timing_disabled()?
Same for others, too.
pvital
left a comment
pvital
left a comment
There was a problem hiding this comment.
Overall, it looks good to me - just a few comments based on @CagriYonca's review.
Why
Some users do not want the Instana
Server-Timingheader (intid;desc=<trace id>) added to their responses and outgoing requests. This adds a documented switch to turn it off, following the same configuration precedence as the other tracing options.What
disable_server_timingtoBaseOptions(defaultFalse). Precedence:INSTANA_TRACING_DISABLE_SERVER_TIMING>INSTANA_CONFIG_PATHYAML (tracing.global.disable-server-timing) > agent config > default. The agent config is applied inStandardOptions._apply_agent_disable_server_timing().get_disable_server_timing_from_yaml()inutil/config.py. It supports the deprecatedcom.instana.tracingroot and tolerates an emptytracing:section.BasePropagator.is_server_timing_disabled(). It reads the flag on every call, so runtime agent-config changes are honoured. It falls back to enabled and logs at debug level if the agent is unavailable.HTTPPropagator,TextPropagator,BinaryPropagatorandlambda_inst.pyskip the header when the flag is set. The Lambda status-code handling is unchanged. The header logic moved into anadd_server_timing_header()helper.TextPropagatorandBinaryPropagator, build the headers once and write them per carrier type, instead of repeatingif server_timing:in every branch. Injected headers and their order are unchanged.is_server_timing_disabled().tests/instrumentation/aws/test_lambda_inst.pywith a mocked agent.