-
Notifications
You must be signed in to change notification settings - Fork 92
Initial tracing #15
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
Initial tracing #15
Conversation
06c7ec2
to
298ff84
Compare
import com.amazonaws.xray.AWSXRay; | ||
import com.amazonaws.xray.entities.Entity; | ||
|
||
public final class PowerTracer { |
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.
Helper utils for setting annotation and metadata on current segment
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.
Can we directly do a good Javadoc documentation?
for info, we will extract all javadoc in order to generate our own documentation website and make it publicly available
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 belive its too early to spend effort on writing java docs. IMO we can delay it a bit until we have a but more structure
} | ||
|
||
// TODO enrich to check more like inherited class | ||
private boolean placedOnHandlerMethod(ProceedingJoinPoint pjp) { |
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 can enrich this further like its done in logging part, but we can do that once we agree on approach here.
|
||
segment = AWSXRay.beginSubsegment("## " + pjp.getSignature().getName()); | ||
|
||
if (placedOnHandlerMethod(pjp)) { |
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 follow the feature from python powertools
Few more things that we should discuss:
Following what we have here https://docs.aws.amazon.com/xray/latest/devguide/scorekeep-workerthreads.html, I cant see how we can improve this experience any further if we have to keep traces under same parent segment.
|
044624b
to
d856068
Compare
d856068
to
120fd61
Compare
import com.amazonaws.xray.entities.Subsegment; | ||
|
||
public final class PowerTracer { | ||
public static final String SERVICE_NAME = null != System.getenv("POWERTOOLS_SERVICE_NAME") |
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.
In the python impl this is "service_undefined" by default. Might be worth matching their project for consistency.
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.
Xray recorder lib by default uses:
private static final String DEFAULT_METADATA_NAMESPACE = "default";
But i can update it to match with that of powertools.
e153307
to
685a00a
Compare
685a00a
to
3df9880
Compare
@@ -25,6 +23,9 @@ | |||
|
|||
import static java.util.Optional.empty; | |||
import static java.util.Optional.of; | |||
import static software.aws.lambda.internal.LambdaHandlerProcessor.isHandlerMethod; | |||
import static software.aws.lambda.internal.LambdaHandlerProcessor.placedOnRequestHandler; | |||
import static software.aws.lambda.internal.LambdaHandlerProcessor.placedOnStreamHandler; | |||
|
|||
@Aspect | |||
public final class LambdaAspect { |
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.
Can this be renamed to LambdaLoggingAspect
|
||
@Aspect | ||
public final class LambdaTracingAspect { | ||
static Boolean IS_COLD_START = null; |
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.
Can we have the cold start logic in a single place?
c45e6cc
to
32d61a0
Compare
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 to me, only need more Javadoc things in order to meet high standard on our code.
I do agree we need to have java doc, but at the same time i feel we should delay it a bit to avoid rework coz of some of the refactoring etc that we might end up doing. |
This is initial suggestion on implementing tracing like power tool.
This PR is meant to start discussion on different aspect of tracing support that we want to have.
PS: test cases are deferred until we decide on approach