Skip to content

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

Merged
merged 6 commits into from
Aug 7, 2020
Merged

Initial tracing #15

merged 6 commits into from
Aug 7, 2020

Conversation

pankajagrawal16
Copy link
Contributor

@pankajagrawal16 pankajagrawal16 commented Aug 4, 2020

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

import com.amazonaws.xray.AWSXRay;
import com.amazonaws.xray.entities.Entity;

public final class PowerTracer {
Copy link
Contributor Author

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

Copy link
Contributor

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

Copy link
Contributor Author

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) {
Copy link
Contributor Author

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)) {
Copy link
Contributor Author

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

@pankajagrawal16
Copy link
Contributor Author

pankajagrawal16 commented Aug 4, 2020

Few more things that we should discuss:

  • How to support tracing of segment for worker threads ? One options is:
   Entity traceEntity = AWSXRay.getTraceEntity();
            Thread comm = new Thread(() -> {
                AWSXRay.setTraceEntity(traceEntity);
                log();
            });

    @PowerToolTracing
    private void log() {
       log.info("inside threaded logging");
    }

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.

  • Should annotation give flexibility of creating new Trace entity ?
  • How can we autopatch or instrument client which is done as part of python power tool ? For java it has to be client specific and has to be left upto users ? Like this https://docs.aws.amazon.com/xray/latest/devguide/xray-sdk-java-awssdkclients.html ?. Should we provide util methods that can give user XRay instrumented clients with some default configs ?

@pankajagrawal16
Copy link
Contributor Author

pankajagrawal16 commented Aug 4, 2020

Sample example from XRay

image

image

image

@pankajagrawal16 pankajagrawal16 force-pushed the initial-tracing branch 4 times, most recently from 044624b to d856068 Compare August 4, 2020 14:36
import com.amazonaws.xray.entities.Subsegment;

public final class PowerTracer {
public static final String SERVICE_NAME = null != System.getenv("POWERTOOLS_SERVICE_NAME")
Copy link
Contributor

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.

Copy link
Contributor Author

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.

@@ -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 {
Copy link
Contributor

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;
Copy link
Contributor

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?

Copy link
Contributor

@stevehouel stevehouel left a 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.

@pankajagrawal16
Copy link
Contributor Author

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.

@pankajagrawal16 pankajagrawal16 merged commit f7007fc into master Aug 7, 2020
@pankajagrawal16 pankajagrawal16 deleted the initial-tracing branch August 7, 2020 12:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants