Conversation
CagriYonca
force-pushed
the
internal-spans
branch
2 times, most recently
from
36fd507 to
a06c130
Compare
CagriYonca
force-pushed
the
internal-spans
branch
from
a06c130 to
1256757
Compare
src/instana/options.py
Outdated
| if "INSTANA_ALLOW_INTERNAL_SPANS" not in os.environ: | ||
| self.add_internal_span_filter() |
There was a problem hiding this comment.
Hey @CagriYonca, the overall structure looks good to me!
But can you explain the significance of this block? Its a bit unclear to me
There was a problem hiding this comment.
I've included this block in case the customer wants to see internal spans.
There was a problem hiding this comment.
I think we aren't supposed to refer this as INTERNAL spans because that could mean the concept of INTERMEDIATE (neither ENTRY nor EXIT but useful/ interesting internal application code) spans. After some contemplating, I remember something that we discussed earlier about whether we should not capture those internal tracer calls at all (existing code) or capture these urllib3/ requests calls to the agent and filter later (this new code).
Although the latter looks more structured I think functionality-wise the earlier could be better because we might try to communicate to the agent multiple times and creating a span each time might be expensive and not something the end-user would be interested (to view with an env var like INSTANA_ALLOW_INTERNAL_SPANS ).
I would like to know the team's thoughts on this, TIA!
There was a problem hiding this comment.
I agree with Varsha that earlier filtering would improve performance by eliminating this specific type of span. However, the task is clear when requested to use the new span filtering feature, and we already have a rejected PR that proposed not collecting these spans earlier.
That said, let's keep this PR but:
- remove this environment variable checking - users don't need to know about these spans, and
- substitute the
add_internal_span_filter()method name with_add_instana_agent_span_filter().
CagriYonca
changed the title
CagriYonca
force-pushed
the
internal-spans
branch
3 times, most recently
from
5810fa7 to
27a05d9
Compare
…ltering mechanism Signed-off-by: Cagri Yonca <cagri@ibm.com>
CagriYonca
force-pushed
the
internal-spans
branch
from
27a05d9 to
f583dd9
Compare
|
| return span_filters | ||
| else: | ||
| return [] | ||
| return {} |
There was a problem hiding this comment.
The "default" return of parse_filtered_endpoints() is a list, so I think it's interesting to keep the same here.
|
|
||
| self.set_disable_trace_configurations() | ||
| self.set_stack_trace_configurations() | ||
| self._add_instana_agent_span_filter() |
There was a problem hiding this comment.
Reviewing it better, I think you must keep the same mindset as before.
Instead of calling the (previously requested by me) _add_instana_agent_span_filter() function, call a set_span_filter_configurations() and this one must call the _add_instana_agent_span_filter() . Just follow the lead of the other "set_*_configurations" functions.
3 participants
No description provided.