Skip to content
This repository has been archived by the owner on Feb 28, 2022. It is now read-only.

feat(logs): Make context dump subdirectory names be more meaningful #249

Merged
merged 3 commits into from
Apr 10, 2019

Conversation

kptdobe
Copy link
Contributor

@kptdobe kptdobe commented Apr 10, 2019

PR for #248

Added the request path inside the folder name (slashes replaced by dashes).

For the following set of requests (in this order):

  1. http://localhost:3000/index.html
  2. http://localhost:3000/docs/ROLLOUT.html
  3. http://localhost:3000/xd/doc/README.html
  4. http://localhost:3000/index.html?debug=true

ls -tl will show the following "context dump" folders:

drwxr-xr-x  19 alex  staff  608 Apr 10 10:14 context_dump_index.default.html_20190310-0814-17.0133_741d12cbea5472965d966dbc5e75d3dc
drwxr-xr-x  19 alex  staff  608 Apr 10 10:14 context_dump_index.html_20190310-0814-16.0591_2dafb5fbea54ff17bfbf18c472da736e
drwxr-xr-x   4 alex  staff  128 Apr 10 10:14 context_dump_xd-doc-README.html_20190310-0814-11.0171_d5eb823d59694513aad62dcddf715647
drwxr-xr-x  19 alex  staff  608 Apr 10 10:14 context_dump_docs-ROLLOUT.default.html_20190310-0814-08.0245_9d3e9c8c91400844b336d34d0b2f5ef8
drwxr-xr-x  19 alex  staff  608 Apr 10 10:14 context_dump_docs-ROLLOUT.html_20190310-0814-07.0167_b83e7905c248c49dcd8de4d026a6ba41
drwxr-xr-x  19 alex  staff  608 Apr 10 10:13 context_dump_index.default.html_20190310-0813-54.0728_79d3de3d4381ca9fa3f40943db07f50b
drwxr-xr-x  19 alex  staff  608 Apr 10 10:13 context_dump_index.html_20190310-0813-53.0840_f5e4ebdb1d0d0749f33a4b7229a1acc3

@codecov
Copy link

codecov bot commented Apr 10, 2019

Codecov Report

Merging #249 into master will not change coverage.
The diff coverage is 100%.

Impacted file tree graph

@@          Coverage Diff          @@
##           master   #249   +/-   ##
=====================================
  Coverage     100%   100%           
=====================================
  Files          39     39           
  Lines         947    948    +1     
  Branches      196    197    +1     
=====================================
+ Hits          947    948    +1
Impacted Files Coverage Δ
src/utils/dump-context.js 100% <100%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update eb41b41...e2da775. Read the comment docs.

Copy link
Contributor

@filmaj filmaj left a comment

Choose a reason for hiding this comment

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

Beautiful, thanks so much!

@tripodsan
Copy link
Contributor

I would format it differently: <timestamp>-<path>-<uuid> , where timestamp is yyyymmddhhmmssSSS. this way it gets sorted correctly, even without the -t

@kptdobe
Copy link
Contributor Author

kptdobe commented Apr 10, 2019

I thought about that but I think it is better to group by path first. You can use the -t to sort by time.
With your solution, you cannot easily sort by path anymore (well for a shell novice, I am sure there is an advanced trick way to do it)

@rofe
Copy link
Contributor

rofe commented Apr 10, 2019

Huge improvement! But do we really need both the timestamp and the id? I don't think the (activation) id has a lot of meaning in local dev as it is made up by helix-simluator anyway, and I'm a little concerned that we may exceed max path lengths on Windows...

@tripodsan
Copy link
Contributor

I thought about that but I think it is better to group by path first. You can use the -t to sort by time.

but if you have maaany directories, you want the newest at the bottom, I guess. also, using the timestamp first, displays them nicer, as all timestamps have the same length.

lastly, the filetime is only second precise...so it might not be accurate :-)

But do we really need both the timestamp and the id?

I agree with rofe. using a millisecond precise timestamp + path should be unique enough

@kptdobe
Copy link
Contributor Author

kptdobe commented Apr 10, 2019

The id is not a random id for uniqueness, it is the 'x-openwhisk-activation-id'. I assume the idea is that you can somehow get the dumps from Runtime where the id and the timestamp would be useful to debug.

Here is the ll output with the <timestamp>-<path>-<uuid> pattern:

drwxr-xr-x  19 alex  staff  608 Apr 10 11:08 context_dump_20190310-0908-14.0616_index.html_43525c285e94fdb85c99ec49c00c44c7
drwxr-xr-x  19 alex  staff  608 Apr 10 11:08 context_dump_20190310-0908-16.0100_index.default.html_66900726edd83af454552439413b8882
drwxr-xr-x  19 alex  staff  608 Apr 10 11:08 context_dump_20190310-0908-28.0258_docs-ROLLOUT.html_508663bf97d9a3824ba11f1b3e5b6e1b
drwxr-xr-x  19 alex  staff  608 Apr 10 11:08 context_dump_20190310-0908-29.0349_docs-ROLLOUT.default.html_d147af902f5243f11112708f9cf996df
drwxr-xr-x   4 alex  staff  128 Apr 10 11:08 context_dump_20190310-0908-35.0207_xd-doc-README.html_97173e4b3a2c8ae4483dcb653cb1e1bb
drwxr-xr-x  19 alex  staff  608 Apr 10 11:08 context_dump_20190310-0908-37.0037_index.html_f2b3c3a8b5538e7d53579ca4b59aa1ac
drwxr-xr-x  19 alex  staff  608 Apr 10 11:08 context_dump_20190310-0908-37.0329_index.default.html_0eea0298bb4726c036835e747fcf891c

Maybe our customer @filmaj has an opinion on the best format I would love to have ? :)

@kptdobe
Copy link
Contributor Author

kptdobe commented Apr 10, 2019

Actually, now that I looked at the output of the other format, I agree it is better :)

@filmaj
Copy link
Contributor

filmaj commented Apr 10, 2019

Maybe our customer @filmaj has an opinion on the best format I would love to have ? :)

The UUID at the end is garbage to me - doesn't help me much. Neither does the context_dump at the front - if I'm already digging away inside a directory named logs/debug, I know what I'm dealing with and don't need that reminder on every file 😉 . I agree with @tripodsan's comment:

also, using the timestamp first, displays them nicer, as all timestamps have the same length.

To me, timestamp and request path is all I want to know. I suggest dropping context_dump and the activation ID at the end.

@kptdobe
Copy link
Contributor Author

kptdobe commented Apr 10, 2019

Ok. Good customer feedback, even real time, thanks :)

This is what you get now:

drwxr-xr-x  19 alex  staff  608 Apr 10 11:27 20190310-0927-12.0896_index.html
drwxr-xr-x  19 alex  staff  608 Apr 10 11:27 20190310-0927-13.0836_index.default.html
drwxr-xr-x  19 alex  staff  608 Apr 10 11:27 20190310-0927-20.0386_docs-ROLLOUT.html
drwxr-xr-x  19 alex  staff  608 Apr 10 11:27 20190310-0927-21.0467_docs-ROLLOUT.default.html
drwxr-xr-x   4 alex  staff  128 Apr 10 11:27 20190310-0927-25.0041_xd-doc-README.html
drwxr-xr-x  19 alex  staff  608 Apr 10 11:27 20190310-0927-27.0878_index.html
drwxr-xr-x  19 alex  staff  608 Apr 10 11:27 20190310-0927-28.0481_index.default.html

@filmaj
Copy link
Contributor

filmaj commented Apr 10, 2019

Love it, it also immediately tells me in what order the pipelines run, which is a nice improvement too.

@tripodsan tripodsan removed their request for review April 10, 2019 09:33
@kptdobe kptdobe merged commit 5c2cadb into master Apr 10, 2019
@kptdobe kptdobe deleted the context-dump-per-request branch April 10, 2019 11:44
trieloff pushed a commit that referenced this pull request Apr 10, 2019
# [1.3.0](v1.2.4...v1.3.0) (2019-04-10)

### Features

* **logs:** Make context dump subdirectory names be more meaningful ([#249](#249)) ([5c2cadb](5c2cadb))
@trieloff
Copy link
Contributor

🎉 This PR is included in version 1.3.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

ramboz pushed a commit that referenced this pull request Apr 15, 2019
# [1.3.0](v1.2.4...v1.3.0) (2019-04-10)

### Features

* **logs:** Make context dump subdirectory names be more meaningful ([#249](#249)) ([5c2cadb](5c2cadb))
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants