TraceID : Fetching TraceID #6797

Vanshikav123 · 2023-10-12T17:49:16Z

I added CHANGELOG entry for this change.
Change is not relevant to the end user.

Changes

Made changes by adding the TraceID in panel state and fetched the header made a Hover feature for tracing the TraceID

#6774

Vanshikav123 · 2023-10-13T11:33:09Z

Hello @yeya24 Please Review

GiedriusS · 2023-10-14T10:37:08Z

pkg/ui/react-app/src/pages/graph/Panel.tsx

+        // Access the response headers
+        const traceIdHeader = resp.headers.get('X-Thanos-Trace-Id');
+        // Set the traceId state
+        this.setState({ traceId: traceIdHeader });


I think we can just join these two lines together into one (https://github.com/thanos-io/thanos/blob/main/docs/contributing/coding-style-guide.md#avoid-defining-variables-used-only-once) and remove the comments since it is obvious what is happening.

@Vanshikav123 This is still not addressed.

GiedriusS · 2023-10-14T10:38:07Z

pkg/ui/react-app/src/pages/graph/Panel.tsx

+    if (event.target.checked && this.state.traceId) {
+      this.setState({ hoverMessage: `TraceID: ${this.state.traceId}` });
+    } else if (event.target.checked) {
+      this.setState({ hoverMessage: 'TraceID is not available. Enable it in your options.' });


Won't this show up if the option is checked but a query hasn't been executed yet? 🤔

I will look into it more deeply 👍

pkg/ui/react-app/src/pages/graph/Panel.tsx

Vanshikav123 · 2023-10-16T08:51:46Z

Thank you for the reviews i think i should remove the condition checking the force tracing condition and instead check whether the query is executed or not .

Vanshikav123 · 2023-10-17T08:38:51Z

Please Review

yeya24 · 2023-11-02T23:23:52Z

pkg/ui/react-app/src/pages/graph/Panel.tsx

@@ -116,6 +119,9 @@ class Panel extends Component<PanelProps & PathPrefixProps, PanelState> {
      exprInputValue: props.options.expr,
      explainOutput: null,
      analysis: null,
+      traceId: null, // Initialize traceId to null.


Code is self explanatory. We can remove the comment

yeya24 · 2023-11-02T23:23:55Z

pkg/ui/react-app/src/pages/graph/Panel.tsx

@@ -58,6 +58,9 @@ interface PanelState {
  exprInputValue: string;
  analysis: QueryTree | null;
  explainOutput: ExplainTree | null;
+  traceId: string | null; //Include traceId in the state.


Code is self explanatory. We can remove the comment

yeya24 · 2023-11-02T23:24:55Z

It is probably also required for you to run make assets to update pkg/ui/bindata.go file.

yeya24 · 2023-11-04T05:42:39Z

It is probably also required for you to run make assets to update pkg/ui/bindata.go file.

@Vanshikav123 Can you please regenerate the bindata file?

Vanshikav123 · 2023-11-04T06:58:06Z

It is probably also required for you to run make assets to update pkg/ui/bindata.go file.

@Vanshikav123 Can you please regenerate the bindata file?

Done

yeya24 · 2023-11-04T07:21:30Z

--- a/pkg/ui/bindata.go
+++ b/pkg/ui/bindata.go
@@ -376,11 +376,13 @@ var _bindata = map[string]func() (*asset, error){
 // directory embedded in the file by go-bindata.
 // For example if you run go-bindata on data/... and data contains the
 // following hierarchy:
-//     data/
-//       foo.txt
-//       img/
-//         a.png
-//         b.png
+//
+//	data/
+//	  foo.txt
+//	  img/
+//	    a.png
+//	    b.png
+//

CI failed. Probably you need to generate bindata again.

yeya24 · 2023-11-04T07:34:33Z

I am trying to test this feature but I didn't get how to use it. @Vanshikav123 Can you please update the pr description and mention how the UX works? Where will the traceID be displayed?

Vanshikav123 · 2023-11-04T10:44:59Z

I am trying to test this feature but I didn't get how to use it. @Vanshikav123 Can you please update the pr description and mention how the UX works? Where will the traceID be displayed?

Actually i will not be able to provide you with the screenshot because my UI is not responding but i will surely provide you with more information about the PR.

Vanshikav123 · 2023-11-05T12:17:38Z

I am trying to test this feature but I didn't get how to use it. @Vanshikav123 Can you please update the pr description and mention how the UX works? Where will the traceID be displayed?

I everytime run make assets after completion of my ui changes , I don't know why these checks are failing .....and I have edited the description of the PR you can review it .

yeya24 · 2023-11-05T16:24:51Z

Thanks for updating the pr description. I saw the traceID displayed when I hover over the Force Tracing tab.

Some suggestions:

Displaying TraceID as a feature shouldn't be bundled with the Force Tracing checkbox... I would like to see users be able to use it without hovering over it. Even if force tracing is not enabled, Thanos backend can still generate the traceID and have it in the response.
I feel it will be more natural displayed here as part of the stats of the response. WDYT?

Vanshikav123 · 2023-11-05T18:09:00Z

Thanks for updating the pr description. I saw the traceID displayed when I hover over the Force Tracing tab.
Some suggestions:

Displaying TraceID as a feature shouldn't be bundled with the Force Tracing checkbox... I would like to see users be able to use it without hovering over it. Even if force tracing is not enabled, Thanos backend can still generate the traceID and have it in the response.

I feel it will be more natural displayed here as part of the stats of the response. WDYT?

Thank you ! @yeya24 for this suggestion , I think this will look good and more informative , I will implement this soon

yeya24 · 2023-12-02T02:20:26Z

Is this ready for another round of review?

Vanshikav123 · 2023-12-02T10:06:45Z

Is this ready for another round of review?

Sorry for the delay , I will push my changes ASAP and will inform you

Vanshikav123 · 2023-12-10T09:20:18Z

Hello @yeya24 I would like to close this branch and will open a new PR soon because the implementation is different from what i did earlier.

pull-request-size bot added the size/S label Oct 12, 2023

Vanshikav123 changed the title ~~Fetching TraceID~~ TraceID : Fetching TraceID Oct 12, 2023

pull-request-size bot added size/M and removed size/S labels Oct 13, 2023

GiedriusS reviewed Oct 14, 2023

View reviewed changes

yeya24 reviewed Oct 15, 2023

View reviewed changes

pkg/ui/react-app/src/pages/graph/Panel.tsx Outdated Show resolved Hide resolved

yeya24 reviewed Nov 2, 2023

View reviewed changes

pull-request-size bot added size/L and removed size/M labels Nov 4, 2023

Vanshikav123 closed this Dec 7, 2023

Vanshikav123 force-pushed the patch-traceID branch from c808877 to e7aecb4 Compare December 7, 2023 12:03

pull-request-size bot added size/XS and removed size/L labels Dec 7, 2023

Vanshikav123 mentioned this pull request Dec 11, 2023

TraceID : Fetching TraceID #6973

Merged

2 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

TraceID : Fetching TraceID #6797

TraceID : Fetching TraceID #6797

Vanshikav123 commented Oct 12, 2023 •

edited

Loading

Vanshikav123 commented Oct 13, 2023

GiedriusS Oct 14, 2023

yeya24 Nov 2, 2023

GiedriusS Oct 14, 2023

Vanshikav123 Oct 16, 2023

Vanshikav123 commented Oct 16, 2023

Vanshikav123 commented Oct 17, 2023

yeya24 Nov 2, 2023

yeya24 Nov 2, 2023

yeya24 commented Nov 2, 2023

yeya24 commented Nov 4, 2023 •

edited

Loading

Vanshikav123 commented Nov 4, 2023

yeya24 commented Nov 4, 2023

yeya24 commented Nov 4, 2023

Vanshikav123 commented Nov 4, 2023

Vanshikav123 commented Nov 5, 2023

yeya24 commented Nov 5, 2023

Vanshikav123 commented Nov 5, 2023

yeya24 commented Dec 2, 2023

Vanshikav123 commented Dec 2, 2023

Vanshikav123 commented Dec 10, 2023

TraceID : Fetching TraceID #6797

TraceID : Fetching TraceID #6797

Conversation

Vanshikav123 commented Oct 12, 2023 • edited Loading

Changes

Vanshikav123 commented Oct 13, 2023

GiedriusS Oct 14, 2023

Choose a reason for hiding this comment

yeya24 Nov 2, 2023

Choose a reason for hiding this comment

GiedriusS Oct 14, 2023

Choose a reason for hiding this comment

Vanshikav123 Oct 16, 2023

Choose a reason for hiding this comment

Vanshikav123 commented Oct 16, 2023

Vanshikav123 commented Oct 17, 2023

yeya24 Nov 2, 2023

Choose a reason for hiding this comment

yeya24 Nov 2, 2023

Choose a reason for hiding this comment

yeya24 commented Nov 2, 2023

yeya24 commented Nov 4, 2023 • edited Loading

Vanshikav123 commented Nov 4, 2023

yeya24 commented Nov 4, 2023

yeya24 commented Nov 4, 2023

Vanshikav123 commented Nov 4, 2023

Vanshikav123 commented Nov 5, 2023

yeya24 commented Nov 5, 2023

Vanshikav123 commented Nov 5, 2023

yeya24 commented Dec 2, 2023

Vanshikav123 commented Dec 2, 2023

Vanshikav123 commented Dec 10, 2023

Vanshikav123 commented Oct 12, 2023 •

edited

Loading

yeya24 commented Nov 4, 2023 •

edited

Loading