Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

cmd/observe: add a new flag to allow specifying different time formats for timestamps #509

Merged
merged 4 commits into from
Mar 16, 2021

Conversation

rolinh
Copy link
Member

@rolinh rolinh commented Mar 11, 2021

This PR adds a new option to the observe command (--time-format) to allow specifying a different time format for timestamps. The default is unchanged and remains time.StampMilli. The following formats are supported:

      --time-format string   Specify the time format for printing. This option does not apply to the json and jsonpb output type. One of:
                               StampMilli:   Jan _2 15:04:05.000
                               RFC3339:      2006-01-02T15:04:05Z07:00
                               RFC3339Milli: 2006-01-02T15:04:05.999Z07:00
                               RFC3339Micro: 2006-01-02T15:04:05.999999Z07:00
                               RFC3339Nano:  2006-01-02T15:04:05.999999999Z07:00
                               RFC1123Z:     Mon, 02 Jan 2006 15:04:05 -0700
                               (default "StampMilli")

In addition, new formats are supported for the --since and --until flags, namely RFC3339 and RFC3339[milli|micro|nano] and RFC1123Z.

@rolinh rolinh added ⌨️ area/cli Impacts the command line interface of any command in the repository. release-note/minor This PR introduces functionality that users may find relevant to operating Hubble. labels Mar 11, 2021
@kaworu
Copy link
Member

kaworu commented Mar 11, 2021

Test failures are legit https://github.com/cilium/hubble/pull/509/checks?check_run_id=2086620687 e.g.

            	Diff:
            	--- Expected
            	+++ Actual
            	@@ -1,2 +1,2 @@
            	-TIMESTAMP: Jan  1 00:20:34.567
            	+TIMESTAMP: 1970-01-01_00:20:34.567
            	      SOURCE: 1.1.1.1:31793
Test:       	TestPrinter_WriteProtoFlow/dict

@kaworu
Copy link
Member

kaworu commented Mar 11, 2021

Personally, I'm not too surprised that the default human readable format cannot be parsed back to a time.Time. The double-click selection depends on the terminal and it's double-click selection delimiters (AFAIK it's sometimes configurable). What I'm trying to say is that the proposed format doesn't guarantee double-click will select the full "time field" as one can have colon (:) as delimiter.

Overall I feel we loose human readability for a use-case that would be better served using another output format, but I don't feel too strongly either way.

@rolinh
Copy link
Member Author

rolinh commented Mar 11, 2021

Overall I feel we loose human readability for a use-case that would be better served using another output format, but I don't feel too strongly either way.

What about adding a new flag to specify the time format? We could keep the existing one but it would allow using different ones.

@rolinh rolinh changed the title Use a new time format when printing out time information RFC: Use a new time format when printing out time information Mar 11, 2021
@michi-covalent
Copy link
Collaborator

looks good to me overall. a few comments/suggestions.

  • makes sense to add a new flag to specify the timestamp format.
  • i prefer to stick to standard format, for example 1985-04-12T23:20:50Z if we are using rfc3339.

rolinh added 4 commits March 12, 2021 10:32
The standard library only defines RFC3339 (with second precision) and
RFC3339Nano (with nanosecond precision). Introduce 2 variants for
millisecond and microsecond precision.

Signed-off-by: Robin Hahling <robin.hahling@gw-computing.net>
Namely, add support for RFC3339Nano, RFC3339Milli, RFC3339Micro and
RFC1123Z.

Signed-off-by: Robin Hahling <robin.hahling@gw-computing.net>
Signed-off-by: Robin Hahling <robin.hahling@gw-computing.net>
The default behavior is unchanged as time.StampMilli is used by default.

Signed-off-by: Robin Hahling <robin.hahling@gw-computing.net>
@rolinh rolinh force-pushed the pr/rolinh/time-format branch from d60232e to 18a7aef Compare March 12, 2021 10:34
@rolinh
Copy link
Member Author

rolinh commented Mar 12, 2021

@michi-covalent I've completely reword the PR to add a --time-format option and the default layout remains unchanged. PTAL.

@rolinh rolinh requested a review from michi-covalent March 12, 2021 10:40
@rolinh rolinh requested a review from kaworu March 12, 2021 10:40
@rolinh rolinh changed the title RFC: Use a new time format when printing out time information cmd/observe: add a new flag to allow specifying different time formats for timestamps Mar 12, 2021
@maintainer-s-little-helper maintainer-s-little-helper bot added the ready-to-merge This PR has passed all tests and received consensus from code owners to merge. label Mar 15, 2021
@michi-covalent michi-covalent merged commit e015695 into master Mar 16, 2021
@michi-covalent michi-covalent deleted the pr/rolinh/time-format branch March 16, 2021 04:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
⌨️ area/cli Impacts the command line interface of any command in the repository. ready-to-merge This PR has passed all tests and received consensus from code owners to merge. release-note/minor This PR introduces functionality that users may find relevant to operating Hubble.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants