debug: use human readable dates for filenames and improve the tests #10806
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
dnephin
added
the
pr/no-changelog
github-actions
bot
added
the
theme/cli
dnephin
force-pushed
the
dnephin/debug-filenames-2
branch
from
460477f to
6f5698b
Compare
dnephin
commented
Aug 17, 2021
Comment on lines
+78
to
+79
| fs.WithFile("metrics.json", "", fs.MatchAnyFileContent), | ||
| fs.WithFile("consul.log", "", fs.MatchFileContent(validLogFile)), |
There was a problem hiding this comment.
Another possible improvement here might be to ensure the files contain at least one line. We may need to run a goroutine to perform some operations to ensure that some logs and metrics are emitted.
Metrics in particular are difficult to capture because we'd have to wait the 10s interval window, which makes the test a bit slower to run.
markan
reviewed
Aug 17, 2021
dhiaayachi
reviewed
Aug 18, 2021
There was a problem hiding this comment.
Thank you for cleaning this up @dnephin !!
I have a question about the time precision and a suggested cosmetic change.
dnephin
force-pushed
the
dnephin/debug-filenames
branch
from
22bbd7c to
26ef0df
Compare
Use the new WriteJsonFile function to write index.json Remove .String() from time.local() since that is done by %s Remove an unused field.
dnephin
force-pushed
the
dnephin/debug-filenames-2
branch
from
6f5698b to
f8b36d0
Compare
dnephin
removed
the
pr/no-changelog
dnephin
force-pushed
the
dnephin/debug-filenames-2
branch
from
f8b36d0 to
bf97e89
Compare
The unix timestamps that were used make the debug data a little bit more difficult to consume. By using human readable dates we can easily see when the profile data was collected. This commit also improves the test coverage. Two test cases are removed and the assertions from those cases are moved to TestDebugCommand. Now TestDebugCommand is able to validate the contents of all files. This change reduces the test runtime of the command/debug package by almost 50%. It also makes much more strict assertions about the contents by using gotest.tools/v3/fs.
dnephin
force-pushed
the
dnephin/debug-filenames-2
branch
from
bf97e89 to
797ee06
Compare
markan
approved these changes
Aug 18, 2021
|
🍒 If backport labels were added before merging, cherry-picking will start automatically. To retroactively trigger a backport after merging, add backport labels and re-run https://circleci.com/gh/hashicorp/consul/430133. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Branched from #10804
The unix timestamps that were used previously make the debug data a little bit more difficult to consume. By using human readable dates we can easily see when the profile data was collected.
Also moves
profile.prof,trace.out,consul.log, andmetrics.jsonout of the sub-directories. There is only one of each of these, so we can put them in the top-level directory.Also improves the test coverage. Two test cases are removed and the assertions from those cases are moved to
TestDebugCommand.Now
TestDebugCommandis able to validate the contents of all files. This change reduces the test runtime of the command/debug package by almost 50%, while also improving the test coverage by making much more strict assertions about the files and their contents.