-
Notifications
You must be signed in to change notification settings - Fork 32
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[FUP Optimisation] redis: added endpoint filtering #688
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Few change requests.
src/instana/agent/host.py
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please convert all message strings to an f-string message for this file.
try: | ||
ignore_endpoints = [] | ||
|
||
# If set from env variable (string format) | ||
if isinstance(params, str): | ||
if not params: | ||
return ignore_endpoints |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can move the if not params:
block out of the try-except block and improve it. BTW, if you expect an empty (null or None) params
, the type annotation must be params: Optional[Union[Dict[str, Any], str]] = None
try: | |
ignore_endpoints = [] | |
# If set from env variable (string format) | |
if isinstance(params, str): | |
if not params: | |
return ignore_endpoints | |
ignore_endpoints = [] | |
if not params: | |
return ignore_endpoints | |
try: | |
# If set from env variable (string format) | |
if isinstance(params, str): | |
... | |
except Exception as e: | |
print(f"Error parsing ignored endpoints: {str(e)}") | |
return ignore_endpoints |
instrumentation = pair.strip() | ||
|
||
if not instrumentation: | ||
continue |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a bit confusing because the same variable name is used in different context blocks. You can move these lines to the else
block and change the variable name to service
instead.
if not instrumentation: | ||
continue | ||
|
||
if ":" in pair: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if ":" in pair: | |
if pair.contains(":"): |
continue | ||
|
||
if ":" in pair: | ||
instrumentation, endpoints = pair.split(":", 1) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Better change the instrumentation
variable name to service
.
instrumentation, endpoints = pair.split(":", 1) | |
service, endpoints = pair.split(":", 1) |
elif isinstance(params, dict): | ||
for service, endpoints in params.items(): | ||
if not endpoints: # If endpoints list is empty | ||
ignore_endpoints.append(service) | ||
else: | ||
for endpoint in endpoints: | ||
ignore_endpoints.append(f"{service}.{endpoint}") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's practice the Clean Code
here. It would be better to split this whole function in two: one to parse string ignored endpoints and another to parse dictionary ignored endpoints.
if check_if_ignored("redis", args[0]): | ||
return |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's prevent the load and import of all these classes. Move this if block out of the try-except block and all the imports (between lines 8 and 10) to below the import redis
.
if "tracing" in config: | ||
if "ignore_endpoints" in config["tracing"]: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if "tracing" in config: | |
if "ignore_endpoints" in config["tracing"]: | |
if "tracing" in config and "ignore_endpoints" in config["tracing"]: |
def check_if_ignored( | ||
instrumentation: str, | ||
command: str, | ||
) -> bool: | ||
"""Check if the given instrumentation and command combination should be ignored.""" | ||
return ( | ||
instrumentation in agent.options.ignore_endpoints | ||
or f"{instrumentation}.{command.lower()}" in agent.options.ignore_endpoints | ||
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A better name for this method is is_service_or_endpoint_ignored
; the variable names must be coherent.
def check_if_ignored( | |
instrumentation: str, | |
command: str, | |
) -> bool: | |
"""Check if the given instrumentation and command combination should be ignored.""" | |
return ( | |
instrumentation in agent.options.ignore_endpoints | |
or f"{instrumentation}.{command.lower()}" in agent.options.ignore_endpoints | |
) | |
def is_service_or_endpoint_ignored( | |
service: str, | |
endpoint: str, | |
) -> bool: | |
"""Check if the given service and endpoint combination should be ignored.""" | |
return ( | |
service in agent.options.ignore_endpoints | |
or f"{service}.{endpoint.lower()}" in agent.options.ignore_endpoints | |
) |
Signed-off-by: Cagri Yonca <[email protected]>
aebd861
to
9b94924
Compare
Added endpoint filtering for redis instrumentation. We've aligned with Java and NodeJS teams. The customer has three options to filter endpoints:
If the customer used more than one way of these three, we'd be using one according to this priority:
Ignoring endpoints through env variable
The customer can use filtering with the environment variable below. If the env variable is set, other two ways will be ignored.
INSTANA_IGNORE_ENDPOINTS="service:endpoint"
INSTANA_IGNORE_ENDPOINTS="service:endpoint1,endpoint2"
INSTANA_IGNORE_ENDPOINTS="service"
INSTANA_IGNORE_ENDPOINTS="service1:endpoint1;service2:endpoint1"
INSTANA_IGNORE_ENDPOINTS="service1;service2"
Ignoring endpoints through the configurator.py file
The customer can add a configuration line like below:
config["tracing"]["ignore_endpoints"] = {"service1": ["endpoint1", "endpoint2"], "service2": []}
Ignoring endpoints through the agent configuration
The customer can add a configuration to agent-folder/etc/instana/configuration.yaml
Reference: https://github.ibm.com/instana/technical-documentation/tree/master/tracing/specification#ignoring-endpoints