feat: add link to logs #620

ace-n · 2022-08-16T01:43:07Z

Fixes #605

ace-n · 2022-08-16T01:43:39Z

UI example

ace-n · 2022-08-16T18:31:14Z

website/templates/donations/view-donation.html

@@ -48,6 +48,17 @@
          <span class="mdc-list-item__meta">{{ donation.time_created }}</span>
        </li>
      </ul>
+    </div>
+    <!-- Buttons -->
+    <div class="emblem-campaign-details mdc-layout-grid__cell mdc-layout-grid__cell--span-12">


Not the cleanest frontend implementation (it doesn't use Material UI's grid layout for right-alignment), but I'm assuming that's not super-important here. 🙂

website/templates/donations/view-donation.html

ace-n · 2022-08-17T00:48:50Z

Update: I tried to get trace ID propagation working for both POST /donate and GET /viewDonations.

I was able to specify the X-Cloud-Trace-Context header in the response, but this has no effect on the request's actual trace ID (as recorded by Cloud Logging).

Looking at this (broken?) code, I think the issue is that Cloud Run expects this as a request header - not a response one.

This is problematic for server-side frameworks (a la Flask), as browsers themselves cannot set headers without additional clientside JS.

@grayside @engelke thoughts?

pattishin · 2022-08-17T02:27:26Z

website/views/donations.py

+    logs_url = None
+    try:
+        trace_id = request.headers.get('X-Cloud-Trace-Context').split('/')[0]
+        logs_url = f'https://console.cloud.google.com/logs/query;query=trace%3D~%22projects%2F.*%2Ftraces%2F{trace_id}%22' if trace_id else None


Confirmed that it points to default logging and default GCP project.

Could we add the following in this query?

project: GCP project id as defined in their PROD_PROJ / STAGE_PROJECT / OPS_PROJECT

resource.type

resource.labels.service_name

resource.labels.location

Current view when I was redirected to my default GCP:

I'm not opposed to resource.type; we can hard-code that.

project and resource.labels.location would require external HTTP queries (in this case, to the Metadata server).

Ditto for service_name, according to this SO post.

To me, this feels like overkill given that the X-Cloud-Trace-Context filters out unrelated "cruft". My only concern is that the "default GCP project" would be a different one than the Emblem app is running on.

Maybe we specify project alone as a middle ground, particularly if it's already included somewhere as an env. var?

Exactly my concern for default project that is would be different from where they have emblem. That's the reason for asking to specify project in the query and having the "View Logs" button direct the user to the correct website container should only be helpful not overkill. If more back & forth is necessary, let's move this conversation offline.

Not sure what's meant by an "external" query, the metadata server is a local service only available inside the sandbox of each Cloud Run container instance.

I think it would be good to have the GCP project available as a variable in the frontend for this and other debug tooling along these lines.

Service Name is available via an environment variable.

In terms of the query that I'd prefer to have in linking to logs, I'd rather not overly filter it: we know that if we filter to a trace ID, we will get all logs across all services and regions that are related to how that request was processed. That comprehensive view is part of the goal.

I will write a separate comment to address the complexity around trace headers.

We're using project ID and trace ID now.

@pattishin is that sufficiently scoped?

pattishin · 2022-08-17T16:54:22Z

this (broken?) code

Jumping in. If this is not working as expected, will add this as a bug issue for this sprint since this is out of context for this PR. Will take responsibility for this as I added that line in.

Update: Issue added #625

grayside · 2022-08-17T17:41:12Z

This is a reply to #620 (comment)

It is possible to specify the trace ID on inbound HTTP requests, and parse that. However, unless our goal is to force Cloud Tracing collection, I don't see a need to customize that header at all. (In the future, we may want to add a force attribute to ensure we consistently have interesting trace data.)

For now, what I imagine is needed is that the Cloud Run service parses the trace context set automatically by Cloud Run on the inbound request, extracts the ID, and uses that to template a URL linking to the Cloud Logging viewer. We already have trace propagation in how that ID is forwarded to the outbound request to the API service, so by extracting it in the website and constructing the URL it will pick up everything needed for all Cloud Run logs on the request.

ace-n · 2022-08-17T21:40:17Z

This is working now; the log entries look like this (without fixing trace propagation):

(I'm assuming trace propagation will be handled in #625, and is outside the scope of this PR.)

grayside · 2022-08-22T18:28:54Z

website/views/donations.py

+        project_id = project_req.text
+
+        if trace_id and project_id:
+            logs_url = f"https://console.cloud.google.com/logs/query;query=trace%3D~%22projects%2F{project_id}%2Ftraces%2F{trace_id}%22?project={project_id}"


Do we need to query limit to project when we're already operating within a particular project?

Good call-out: TIL you can do cross-project traces.

@pattishin do you mind if we drop the per-project filter?

grayside · 2022-08-22T18:37:57Z

website/views/donations.py

@@ -119,8 +121,24 @@ def webapp_view_donation():
        log(f"Exception when getting a campaign for a donations: {e}", severity="ERROR")
        return render_template("errors/403.html"), 403

+    logs_url = None
+    try:
+        trace_id = request.args.get("trace_id")


Not sure how this gets the trace_id, it's not injected as a query string parameter? You can see how trace is extracted in https://github.com/GoogleCloudPlatform/emblem/blob/main/website/middleware/logging.py
and https://github.com/GoogleCloudPlatform/emblem/blob/32cb6756db291407245918f14e1f900473fd92db/website/middleware/auth.py

See line 106 - the POST /donate trace ID is captured there, and forwarded to the GET /viewDonation request via this query parameter.

(As explained here, the trace ID from POST /donate does not get propagated to GET /viewDonation. Thus, they have totally separate trace IDs.)

We can talk more broadly about the trace propagation in #625.

I'm okay not blocking this, but maybe we should have a TODO pointing to 625 for a more permanent solution?

For this specific mode on trace propagation, I don't like the idea that the trace_id is a visible part of the website interface. This may make more sense as session data. @muncus do you have any thoughts on "manual" trace propagation across a web redirect?

@grayside now that you mention it, cookies seem appropriate here. @engelke are we OK using those instead of session buckets?

(I'd rather not write to GCS with each request. 🙂 )

Rationale for cookies: this is ephemeral, user-specific data

Is there a specific reason the two requests should share a trace id? My understanding is that traces are intended to model single interactions, and trace IDs are not intended to be reused between requests (hence why you're having a hard time doing so 😄 ).

If displaying the donation really must be in the same trace, i'd suggest rendering the template here directly, instead of redirecting. Otherwise i'd let the two actions remain separate.

@muncus this is useful context, thanks. We could include both trace IDs in the log filter, perhaps?

grayside

There are a couple discussions in progress but this LGTM with follow-ups as needed.

pattishin · 2022-08-24T21:15:54Z

Adding the follow up topics in our next sprint planning meeting notes so the discussion can continue there.

ace-n · 2022-08-24T21:35:29Z

@grayside Patti and I decided to punt the cookies-vs-URL-args discussion to a subsequent issue/PR. 👍

(LMK if you'd like me to file that!)

feat: add link to logs

9c69198

ace-n requested a review from pattishin August 16, 2022 01:43

ace-n requested a review from a team as a code owner August 16, 2022 01:43

github-actions bot added component: website Related to the application frontend. type: feature request ‘Nice-to-have’ improvement, new feature or different behavior or design. labels Aug 16, 2022

ace-n self-assigned this Aug 16, 2022

ace-n commented Aug 16, 2022

View reviewed changes

grayside reviewed Aug 16, 2022

View reviewed changes

website/templates/donations/view-donation.html Outdated Show resolved Hide resolved

Timestamps -> trace IDs

468f002

github-actions bot added type: feature request ‘Nice-to-have’ improvement, new feature or different behavior or design. and removed type: feature request ‘Nice-to-have’ improvement, new feature or different behavior or design. labels Aug 16, 2022

Merge branch 'main' into logs-btn

afb63dc

github-actions bot added type: feature request ‘Nice-to-have’ improvement, new feature or different behavior or design. and removed type: feature request ‘Nice-to-have’ improvement, new feature or different behavior or design. labels Aug 17, 2022

pattishin suggested changes Aug 17, 2022

View reviewed changes

pattishin mentioned this pull request Aug 17, 2022

feat: debug link from frontend to cloud console #605

Closed

Merge branch 'main' into logs-btn

3323a65

github-actions bot added type: feature request ‘Nice-to-have’ improvement, new feature or different behavior or design. and removed type: feature request ‘Nice-to-have’ improvement, new feature or different behavior or design. labels Aug 17, 2022

pattishin mentioned this pull request Aug 17, 2022

Bug: Resolve bug that is possibly preventing trace ID propagation on viewing donations #625

Open

ace-n added 2 commits August 17, 2022 14:36

Get trace IDs semi-working

3ebdc78

Specify project ID

b2ef895

github-actions bot added type: feature request ‘Nice-to-have’ improvement, new feature or different behavior or design. and removed type: feature request ‘Nice-to-have’ improvement, new feature or different behavior or design. labels Aug 17, 2022

fix lint

e0017f7

github-actions bot added type: feature request ‘Nice-to-have’ improvement, new feature or different behavior or design. and removed type: feature request ‘Nice-to-have’ improvement, new feature or different behavior or design. labels Aug 17, 2022

ace-n requested a review from pattishin August 17, 2022 23:25

Merge branch 'main' into logs-btn

f0214eb

github-actions bot added type: feature request ‘Nice-to-have’ improvement, new feature or different behavior or design. and removed type: feature request ‘Nice-to-have’ improvement, new feature or different behavior or design. labels Aug 19, 2022

ace-n removed their assignment Aug 19, 2022

grayside reviewed Aug 22, 2022

View reviewed changes

Merge branch 'main' into logs-btn

8987c1a

github-actions bot added type: feature request ‘Nice-to-have’ improvement, new feature or different behavior or design. and removed type: feature request ‘Nice-to-have’ improvement, new feature or different behavior or design. labels Aug 22, 2022

Merge branch 'main' into logs-btn

2ae2621

github-actions bot added type: feature request ‘Nice-to-have’ improvement, new feature or different behavior or design. and removed type: feature request ‘Nice-to-have’ improvement, new feature or different behavior or design. labels Aug 23, 2022

grayside approved these changes Aug 23, 2022

View reviewed changes

Merge branch 'main' into logs-btn

0ff835b

github-actions bot added type: feature request ‘Nice-to-have’ improvement, new feature or different behavior or design. and removed type: feature request ‘Nice-to-have’ improvement, new feature or different behavior or design. labels Aug 24, 2022

pattishin approved these changes Aug 24, 2022

View reviewed changes

ace-n merged commit bab1ab7 into main Aug 24, 2022

ace-n deleted the logs-btn branch August 24, 2022 21:35

github-actions bot mentioned this pull request Jan 26, 2024

chore(main): release emblem 1.0.0 #732

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: add link to logs #620

feat: add link to logs #620

ace-n commented Aug 16, 2022

ace-n commented Aug 16, 2022

ace-n Aug 16, 2022 •

edited

Loading

ace-n commented Aug 17, 2022 •

edited

Loading

pattishin Aug 17, 2022 •

edited

Loading

ace-n Aug 17, 2022 •

edited

Loading

pattishin Aug 17, 2022 •

edited

Loading

grayside Aug 17, 2022

ace-n Aug 17, 2022

pattishin commented Aug 17, 2022 •

edited

Loading

grayside commented Aug 17, 2022

ace-n commented Aug 17, 2022 •

edited

Loading

grayside Aug 22, 2022

ace-n Aug 22, 2022

grayside Aug 22, 2022

ace-n Aug 22, 2022

grayside Aug 23, 2022 •

edited

Loading

ace-n Aug 24, 2022 •

edited

Loading

muncus Aug 24, 2022

ace-n Aug 24, 2022

grayside left a comment

pattishin commented Aug 24, 2022

ace-n commented Aug 24, 2022

feat: add link to logs #620

feat: add link to logs #620

Conversation

ace-n commented Aug 16, 2022

ace-n commented Aug 16, 2022

ace-n Aug 16, 2022 • edited Loading

Choose a reason for hiding this comment

ace-n commented Aug 17, 2022 • edited Loading

pattishin Aug 17, 2022 • edited Loading

Choose a reason for hiding this comment

ace-n Aug 17, 2022 • edited Loading

Choose a reason for hiding this comment

pattishin Aug 17, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pattishin commented Aug 17, 2022 • edited Loading

grayside commented Aug 17, 2022

ace-n commented Aug 17, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

grayside Aug 23, 2022 • edited Loading

Choose a reason for hiding this comment

ace-n Aug 24, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

grayside left a comment

Choose a reason for hiding this comment

pattishin commented Aug 24, 2022

ace-n commented Aug 24, 2022

ace-n Aug 16, 2022 •

edited

Loading

ace-n commented Aug 17, 2022 •

edited

Loading

pattishin Aug 17, 2022 •

edited

Loading

ace-n Aug 17, 2022 •

edited

Loading

pattishin Aug 17, 2022 •

edited

Loading

pattishin commented Aug 17, 2022 •

edited

Loading

ace-n commented Aug 17, 2022 •

edited

Loading

grayside Aug 23, 2022 •

edited

Loading

ace-n Aug 24, 2022 •

edited

Loading