feat(logcli): Add timestamp format flag via cli argument for default output

[chu121su12]

What this PR does / why we need it:

This PR will address a request to add optional subsecond timings to a logcli command line request. By adding a --timestamp=nanos you will output logs in RFC3339Nano instead of RFC3339 for the default output only.

Which issue(s) this PR fixes:
Fixes test in #15189 that is adding feature to close #9635. The --timestamp flag is optional and by default still uses the existing format of RFC3339

Special notes for your reviewer:
The original PR is not yet updated by the author @steelrain74 and I'd want to propose update while also testing it against github CI. My local test passes only for the check / testPackages (pkg/logcli) section

Checklist

• Reviewed the CONTRIBUTING.md guide (required)
• Documentation added
• Tests updated
• Title matches the required conventional commits format, see here
  • Note that Promtail is considered to be feature complete, and future development for logs collection will be in Grafana Alloy. As such, feat PRs are unlikely to be accepted unless a case can be made for the feature actually being a bug fix to existing behavior.
• Changes that require user attention or interaction to upgrade are documented in docs/sources/setup/upgrade/_index.md
• If the change is deprecating or removing a configuration option, update the deprecated-config.yaml and deleted-config.yaml files respectively in the tools/deprecated-config-checker directory. Example PR

[CLAassistant]

All committers have signed the CLA.

[trevorwhitney]

I like this idea. I'm curious if we could try to do a better job of automatically detecting the timestamp type? How far could we get without introducing a new argument?

Also, I like to see some tests for this new functionality.

Thanks!

[trevorwhitney]

could you add some tests around the additional cases you've introduced?

[chu121su12]

Does adding some of the allowed values enough or any pointer on what should I test?

loki/cmd/logcli/main.go

Line 36 in 1cce668

timestamp = app.Flag("timestamp", "Specify the format of timestamps in the default output mode [seconds, nanos, unix, rfc822z, rfc1123z, stampmilli, stampmicro, stampnano]").Default("seconds").Enum("seconds", "nanos", "unix", "rfc822z", "rfc1123z", "stampmillis", "stampmicros", "stampnanos")

[chu121su12]

Added 2 tests within the following as additional permutations:

loki/pkg/logcli/output/default_test.go

Line 14 in e8724f7

func TestDefaultOutput_Format(t *testing.T) {

To note, all other LogOutputOptions{} invocations does it with named argument and this is the only instance it is constructed not as named argument (sorry my terminology) so I assume it should not be used as such and changed the test construction to follow the rest of the codebase.

Also at the place it is being used, there is fallback to the default formatter to preserve compatibility

loki/pkg/logcli/output/default.go

Lines 22 to 25 in e8724f7

	format := o.options.TimestampFormat
	if format == "" {
	format = time.RFC3339
	}

[trevorwhitney]

I like this feature, but I have some feedback on the arguments. nanos as RFC3339Nano is confusing to me, as the Loki API accepts a nanosecond epoch time as well, which is what I guess in this case would actually be a unix timestamp, but I think that will trip people up. I rather be more explicit.

Furthermore, could we add some logic for detecting the timestamp? For example, when parsing the timestamp in Loki we automatically detect the timestamp type. Not sure if we can capture all of these, but I think we could capture a bunch, wdyt?

[chu121su12]

I like this idea. I'm curious if we could try to do a better job of automatically detecting the timestamp type? How far could we get without introducing a new argument?

This is an output timestamp so it should be provided, not detected. Maybe change in arg name/parameter?

[chu121su12]

be more explicit

updated in latest commit to use the lowercased time.XXX name instead

[trevorwhitney]

yeah, if we want to keep this as just an output change, let's change the parameter to indicate that. however, it would be neat if we could accept more formats for the input timestamps as well, in which case the auto-detection would be neat, but maybe outside the scope of this PR? where I see the overlap is we could default the output format to match the input unless overridden?

[chu121su12]

What would you suggest the arg name? For its value should there be support for all the time.Parse formats (as in --timestamp-layout="xxx")? Should the aliases (rfc, stamp, etc) be dropped?

As for input timestamps, I'm sorry I don't follow for maybe I'm not using loki/logcli enough. But when does logcli expect a non standard (non-loki) input timestamps?

If my guess you are referring to the --from and --to values then I agree it is outside the scope of this PR. (Although the flag name should be more clear since the --timezone is already used for output modifier)

[trevorwhitney]

Yes, I'm referring to --from and --to, as logcli only accepts timestamps as RFC3339Nano When I first saw your PR I was hoping it would deal with that side too, as I find it really annoying that I can't use the same timestamps with logcli as I can in the API. Also, if we went that route, it could be neat to default the output to the format of the input, which could be automatically detected.

If you'd prefer to stick with only addressing output timestamps, we should rename the argument to --output-timestamp or --output-timestamp-format.

[chu121su12]

Latest commit update only the argument name.

This is my incomplete attempt (relevant tests) at detecting from/to format and using it for output formatting. This version would immediately introduces breaking changes due to the man line stating the value should be in "nano without timestamp" but the output formatting (not adjustable) is in time.RFC3339.

loki/cmd/logcli/main.go

Lines 62 to 64 in e3e1f09

	Notice that when using --from and --to then ensure to use RFC3339Nano
	time format, but without timezone at the end. The local timezone will be added
	automatically or if using --timezone flag.

Let me know if you want to pursue this version instead.

[trevorwhitney]

I pushed a commit to rename the variable outputTimestampFmt and include unixdate as an option, which you had in the switch statement but were missing from the cli argument defintion.

Looks great, thanks for this 👍