Skip to content
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

Add SQL to trace #173

Merged
merged 2 commits into from
Feb 6, 2023
Merged

Add SQL to trace #173

merged 2 commits into from
Feb 6, 2023

Conversation

kevinwcyu
Copy link
Contributor

@kevinwcyu kevinwcyu commented Feb 1, 2023

Adds sql information to trace if available.

Changes

Fixes: #142

Note to reviewer: we have a test app set up in our AWS dev account that generates SQL traces. You can use this trace id to see an example with SQL data 1-63d96675-436a86c00a3c0c7d1ae4230f.

sql trace

@github-actions
Copy link

github-actions bot commented Feb 2, 2023

Backend code coverage report for PR #173
No changes

@github-actions
Copy link

github-actions bot commented Feb 2, 2023

Frontend code coverage report for PR #173

Plugin Main PR Difference
src 87.5% 87.5% 0%

@kevinwcyu kevinwcyu marked this pull request as ready for review February 2, 2023 17:57
@kevinwcyu kevinwcyu requested a review from a team as a code owner February 2, 2023 17:57
@@ -177,18 +206,21 @@ export type XrayTraceDataSegmentDocument = {
end_time?: number;
in_progress?: boolean;
// Same as top level Id
trace_id: string;
trace_id?: string;
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Trace ID of the subsegment's parent segment. Required only if sending a subsegment separately.
Subsegment fields

Copy link
Contributor

@idastambuk idastambuk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@fridgepoet
Copy link
Member

fridgepoet commented Feb 3, 2023

I couldn't find the data in the AWS console, but I did see it in Grafana! Just like in your screen shot but of course in light mode XD

If you have any tips on seeing it in AWS, that would be welcome! Edit: facepalm, wrong region

Copy link
Member

@sarahzinger sarahzinger left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks so much for this! a few random thoughts

@@ -110,7 +286,7 @@ const result = new MutableDataFrame({
'1-5ee20a4a-bab71b6bbc0660dba2adab3e',
'1-5ee20a4a-bab71b6bbc0660dba2adab3e',
'1-5ee20a4a-bab71b6bbc0660dba2adab3e',
undefined,
'',
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is this an empty string because it is a subsegment? Or is this just because it's a test and it doesn't really matter?

Copy link
Contributor Author

@kevinwcyu kevinwcyu Feb 6, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Trace ID of the subsegment's parent segment. Required only if sending a subsegment separately.
Subsegment fields

From my understanding of the docs, and from the payload we receive from X-Ray, this can be empty because the trace id is only sent if a subsegment is sent separately, otherwise this field is empty. By sent separately, it's something like this

{
  "ID": "trace-id-1",
  "Segments": [
    {
      "Document": {
        "id": "segment-id-1",
        "trace_id": "trace-id-1"
        "subsegments": [
          // a subsegment of 'segment-id-1'.
          {
            "id": "subsegment-id-1"
            // trace_id is not a part of this subsegment because it doesn't have any subsegments that are sent separately
          }
        ],
      },
    },
    {
      // this is a subsegment of segment-id-1 that is sent separately
      "Document": {
        "id": "subsegment-id-1",
        "trace_id": "trace-id-1",
      }
    }
  ]
}

'1-12345678-1234567890abcdefghijklmn',
'1-12345678-1234567890abcdefghijklmn',
'1-12345678-1234567890abcdefghijklmn',
'',
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same as above here

@@ -50,7 +50,7 @@ export function transformTraceResponse(data: XrayTraceData): DataFrame {
serviceTags,
spanID: segment.Document.name + segment.Document.origin,
startTime: segment.Document.start_time * MS_MULTIPLIER,
traceID: segment.Document.trace_id,
traceID: segment.Document.trace_id || '',
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

would it break the rendering if we said something like || 'subsegment' or something? It might be easier to understand later if we're logging or something what we're looking at maybe?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the spanID can be used to identify the span for logging purposes.

i wouldn't feel confident setting this to anything other than what AWS sends us. the improvement now is that it makes it explicit that the trace_id from AWS could possibly be undefined, which the type didn't reflect previously.

@kevinwcyu kevinwcyu merged commit 46f9723 into main Feb 6, 2023
@kevinwcyu kevinwcyu deleted the kevinwcyu/142 branch February 6, 2023 18:53
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.

Xray Trace does not show SQL on trace in grafana
4 participants