Add transaction_ignore_urls spec#333
Conversation
💔 Build Failed
Expand to view the summary
Build stats
Steps errorsExpand to view the steps failures
Log outputExpand to view the last 100 lines of log output
|
…e_urls * upstream/master: [CI] compare with the calculated SHA commit (elastic#336) add link to PHP documentation Link to create-agent-issues.sh in spec process
| http://whatever.com/home/index?value1=123 | ||
| ``` | ||
|
|
||
| NOTE: |
There was a problem hiding this comment.
What is the motivation for this exception?
There was a problem hiding this comment.
This has always been the case, transaction sampling/ignoring has no effect on exception tracking
| ``` | ||
|
|
||
| NOTE: | ||
| All errors that are captured during a request to an ignored URL are still sent to the APM Server regardless of this setting. |
There was a problem hiding this comment.
Does it mean errors for ignored transactions won’t have parent_id, transaction_id, transaction, context and trace_id?
There was a problem hiding this comment.
transaction_id will obviously have to be set to null or equivalent, I'm not so sure about the others. trace_id could theoretically be around from distributed tracing, but it might involve overhead to parse it and make it available for error collection, which is exactly what we want to avoid with this setting.
In the Python agent, we generate the context for exceptions separately, but that might be considered an implementation detail.
|
|
||
| ##### `transaction_ignore_urls` configuration | ||
|
|
||
| Used to restrict requests to certain URLs from being instrumented. |
There was a problem hiding this comment.
Should agent propagate data related to distributed tracing?
There was a problem hiding this comment.
I guess the simplest thing to do is to treat the ignored URLs as if there's no agent at all. I expect most of the use cases to exclude health checks and static resources. For those cases, typically no downstream services are involved. Obviously, there can be exceptions to that rule.
Does any of our agents propagate the context for ignored URLs?
I'm also fine to not explicitly specify that bit. If it becomes an important question, we can follow up on it.
There was a problem hiding this comment.
If the use case for this feature is to completely exclude parts of the site/application from being monitored (which is what I assumed when I first heard about this feature) than why do we need the exception for errors, discussed a few comments above? I think it would be much cleaner mental model if agent doesn't do anything for ignored URLs - agent doesn't report anything, doesn't propagate distributed tracing data, etc.
There was a problem hiding this comment.
You're probably onto something here. This is how we implemented error handling in the context of processing a request that corresponds to an ignored URL:
When the user manually captures an error we still send that error.
However, we won't catch exceptions that happen during the request processing and automatically capture an error, as we'd do for non-ignored URLs.
This might be different from how other agents handle it. If that's the case, I think it does make sense to align eventually but I'd like to handle that in a follow-up and not block the progress on this spec.
There was a problem hiding this comment.
So wouldn’t it be better to leave both error reporting and distributed tracing data propagation aspects unspecified?
There was a problem hiding this comment.
SGTM, unless all agents currently behave the same anyways. But seems like they don't.
There was a problem hiding this comment.
@SergeyKleyman @felixbarny so if I understand the discussion correctly, the spec is OK as is? Or would you like a note that error handling and trace propagation remains unspecified for now?
There was a problem hiding this comment.
It seems that existing implementations by the agents differ in error handling and trace propagation parts so the simplest approach at this point would be to have those aspects unspecified. I'm not sure a note is necessary - we can just not include those parts in the spec.
|
@elastic/apm-agent-rum can you confirm that you're OK with this spec? |
hmdhk
left a comment
There was a problem hiding this comment.
@beniwohli , We don't have any plans to add this config option to kibana at the moment, but we will align the name with other agents once that happens.
supersedes #144
Note that the implementation of this spec also includes adding the option to Kibana.