-
Notifications
You must be signed in to change notification settings - Fork 174
MLAgentTracer Class #3946
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
MLAgentTracer Class #3946
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.
Left a few comments, I'm seeing the commit history is a criss crossed with some commits can we rebase with the opensearch-project:feature/agent-tracing branch to see if it fixes it?
Also lets make sure to sign off our commits using -s as in git commit -s -m "<your Message>"
you can see this is why DCO is complaining
return tracer; | ||
} | ||
|
||
static void resetForTest() { |
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.
Should we use a @ VisibleForTesting
annotation also what else do we need to reset?
|
||
public static synchronized MLAgentTracer getInstance() { | ||
if (instance == null) { | ||
throw new IllegalStateException("MLAgentTracer is not initialized. Call initialize() first or check feature flag."); |
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 be specific which feature flag needs to be flipped
plugins.ml_commons.agent_tracing_feature_enabled
or
plugins.ml_commons.agent_tracing_enabled
?
As a side note how can I tell what setting I need to switch on?
d4cca33
to
6eb634d
Compare
Signed-off-by: chrislai <[email protected]>
Signed-off-by: chrislai <[email protected]>
Signed-off-by: chrislai <[email protected]>
Signed-off-by: chrislai <[email protected]>
Signed-off-by: chrislai <[email protected]>
6eb634d
to
c712e12
Compare
Signed-off-by: chrislai <[email protected]>
Signed-off-by: chrislai <[email protected]>
Signed-off-by: chrislai <[email protected]>
Signed-off-by: chrislai <[email protected]>
* The returned span should be passed to {@link #endSpan(Span)} when the | ||
* operation completes. | ||
*/ | ||
public abstract Span startSpan(String name, Map<String, String> attributes, Span parentSpan); |
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.
what do you think of creating an overloaded method for spans without parents:
startSpan(String name, Map<String, String> attributes);
This internally can call the startSpan method with null parent
} | ||
SpanCreationContext context = SpanCreationContext.server().name(name).attributes(attrBuilder); | ||
Span newSpan; | ||
if (name != null && name.startsWith(AGENT_TASK_SPAN) && !(tracer instanceof NoopTracer)) { |
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.
any reason for checking if (!tracer instance of NoopTracer)
i would assume since they are both tracer objects, you could simply use the methods but just that internally NoopTracer wouldn't do anything
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.
We check for NoopTracer since although it gracefully handles public methods of Tracer, we use reflection to access internals (for custom span creation and injection/extraction) and NoopTracer doesn't have these so it will give a NoSuchFieldException.
The agent execution will still run as normal, but I'm just checking to avoid the exception.
* @param carrier The map to inject context into. The span context will be added | ||
* as key-value pairs to this map. Must not be null. | ||
*/ | ||
public void injectSpanContext(Span span, Map<String, String> carrier) { |
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.
when will these method be used?
- injectSpanContext
- extractSpanContext
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.
These are used in Plan-Execute-Reflect and Conversational agents to connect the conversational agent span with the p-e-r agent span
Signed-off-by: chrislai <[email protected]>
1abc5fb
into
opensearch-project:feature/agent-tracing
* adding agent tracing to mlplugin Signed-off-by: chrislai <[email protected]> * add settings and clean code Signed-off-by: chrislai <[email protected]> * add tests Signed-off-by: chrislai <[email protected]> * cr fixes Signed-off-by: chrislai <[email protected]> * tests message fix and spotlessApply Signed-off-by: chrislai <[email protected]> * spotlessApply Signed-off-by: chrislai <[email protected]> * add tests Signed-off-by: chrislai <[email protected]> * fix comments Signed-off-by: chrislai <[email protected]> * add visiblefortesting Signed-off-by: chrislai <[email protected]> * address comments Signed-off-by: chrislai <[email protected]> * javadoc Signed-off-by: chrislai <[email protected]> * more tests Signed-off-by: chrislai <[email protected]> * more class fixes Signed-off-by: chrislai <[email protected]> * overloaded method Signed-off-by: chrislai <[email protected]> --------- Signed-off-by: chrislai <[email protected]>
* adding agent tracing to mlplugin Signed-off-by: chrislai <[email protected]> * add settings and clean code Signed-off-by: chrislai <[email protected]> * add tests Signed-off-by: chrislai <[email protected]> * cr fixes Signed-off-by: chrislai <[email protected]> * tests message fix and spotlessApply Signed-off-by: chrislai <[email protected]> * spotlessApply Signed-off-by: chrislai <[email protected]> * add tests Signed-off-by: chrislai <[email protected]> * fix comments Signed-off-by: chrislai <[email protected]> * add visiblefortesting Signed-off-by: chrislai <[email protected]> * address comments Signed-off-by: chrislai <[email protected]> * javadoc Signed-off-by: chrislai <[email protected]> * more tests Signed-off-by: chrislai <[email protected]> * more class fixes Signed-off-by: chrislai <[email protected]> * overloaded method Signed-off-by: chrislai <[email protected]> --------- Signed-off-by: chrislai <[email protected]>
Description
Adds new MLAgentTracer class to capture traces within ML-commons and settings to disable or enable tracing.
Related Issues
Resolves #3971
Check List
--signoff
.By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.