-
Notifications
You must be signed in to change notification settings - Fork 4
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
feat(RD-12769): add execution tags #71
base: master
Are you sure you want to change the base?
Conversation
tag.put("value", normalizeTag(value)); | ||
tags.add(tag); | ||
} catch (Exception err) { | ||
if (shouldLogErrors) { |
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.
Im not sure why you need this condition? you are logging the error either way
@@ -73,4 +75,16 @@ public OUTPUT handleRequest(INPUT input, Context context) { | |||
} | |||
|
|||
public abstract OUTPUT doHandleRequest(INPUT input, Context context); | |||
|
|||
public void addExecutionTag(String key, String value) { |
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.
I talked with Harel and we agreed that we dont want to add this here, lets expose those methods from the ExecutionTags
class.
spansContainer.addExecutionTag(key, value); | ||
} | ||
|
||
public void clearExecutionTags() { |
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.
I think the only method that should be exposed to the user is AddExecutionTag
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.
Good Work, I left you some comments.
Also add some tests please
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 include matching README instructions for users
@@ -73,7 +75,9 @@ public void clear() { | |||
rttDuration = null; | |||
endFunctionSpan = null; | |||
reporter = null; | |||
httpSpans = new LinkedList<>(); |
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.
Why did you add this line?
} | ||
} | ||
|
||
public static List<Map<String, String>> getTags() { |
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.
lets make getTags & clearTags package private methods, I dont want to expose them to users
return (val == null) ? null : String.valueOf(val); | ||
} | ||
|
||
public static void addTag(String key, String value, boolean shouldLogErrors) { |
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.
I dont think we want the shouldLogErrors
did you see it in the other tracers?
here's an example of execution tags in the node tracer: |
No description provided.