Skip to content
This repository was archived by the owner on Sep 30, 2024. It is now read-only.

Outbound request log: Add complete call stack#45151

Merged
vdavid merged 5 commits into
mainfrom
dv/outbound-request-logs-complete-stack-trace
Dec 5, 2022
Merged

Outbound request log: Add complete call stack#45151
vdavid merged 5 commits into
mainfrom
dv/outbound-request-logs-complete-stack-trace

Conversation

@vdavid

@vdavid vdavid commented Dec 5, 2022 •
edited
Loading

Copy link
Copy Markdown
Contributor

We wanted more complete stack traces for our new "Outbound request log" feature to help us figure out where calls were coming from.

This PR adds them:

CleanShot 2022-12-05 at 10 52 23@2x

Notes:

  • I still use a plain text format for the data transmission and parse it on the front end, which is not beautiful, but the effort to make this more sophisticated didn't seem to justify the effort needed.
  • The Redis field should be called "CallStack" rather than "CallStackFrame" now, but I haven't renamed it because that'd need a migration / custom unmarshaling, and I deemed it not worth it at this point because I expect more changes to the data format. I could do a migration once the feature is more mature ("Duration" format also changed, here).
  • Also sneaked in a small front-end improvement to sort the header items alphabetically (was pretty much random)

Test plan

40-sec Loom demo

  • It shows the stack trace
  • It's backward compatible

@vdavid vdavid requested a review from mrnugget December 5, 2022 10:01
@cla-bot cla-bot Bot added the cla-signed label Dec 5, 2022
@sourcegraph-bot

sourcegraph-bot commented Dec 5, 2022 •
edited
Loading

Copy link
Copy Markdown
Contributor

Codenotify: Notifying subscribers in CODENOTIFY files for diff f95cd6a...0ba0654.

Notify File(s)
@keegancsmith internal/httpcli/redis_logger_middleware.go

mrnugget
mrnugget suggested changes Dec 5, 2022
View reviewed changes

@mrnugget mrnugget left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Nice! But I think we need to be careful about wrong links

Comment thread client/web/src/site-admin/SiteAdminOutboundRequestsPage.tsx Outdated
@sg-e2e-regression-test-bob

sg-e2e-regression-test-bob commented Dec 5, 2022 •
edited
Loading

Copy link
Copy Markdown

Bundle size report 📦

Initial size Total size Async size Modules
0.00% (0.00 kb) 0.04% (+6.59 kb) 0.05% (+6.59 kb) 0.00% (0)

Look at the Statoscope report for a full comparison between the commits 0ba0654 and 833cdad or learn more.

Note: We do not have exact data for 8ddaa9b. So we have used data from: 833cdad.
The intended commit has no frontend pipeline, so we chose the last commit with one before it.

Open explanation
  • Initial size is the size of the initial bundle (the one that is loaded when you open the page)
  • Total size is the size of the initial bundle + all the async loaded chunks
  • Async size is the size of all the async loaded chunks
  • Modules is the number of modules in the initial bundle

@vdavid vdavid merged commit 33007c4 into main Dec 5, 2022
@vdavid vdavid deleted the dv/outbound-request-logs-complete-stack-trace branch December 5, 2022 11:32
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

1 more reviewer

@mrnugget mrnugget mrnugget approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

cla-signed

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@vdavid @sourcegraph-bot @sg-e2e-regression-test-bob @mrnugget