New option `timestamp.precision` to control timestamp precision #31682

kvch · 2022-05-19T14:23:07Z

What does this PR do?

This PR makes timestamp precision configurable. A new config option is added called timestamp.precision. Available options are millisecond microseconds and nanosecond. Default value is millisecond.

Why is it important?

We made nanosecond support default in #31553. But such timestamps require more storage. Thus, we are making it opt-in.

Checklist

My code follows the style guidelines of this project
~~- [ ] I have commented my code, particularly in hard-to-understand areas~~
I have made corresponding changes to the documentation
I have made corresponding change to the default configuration files
I have added tests that prove my fix is effective or that my feature works
I have added an entry in CHANGELOG.next.asciidoc or CHANGELOG-developer.next.asciidoc.

elasticmachine · 2022-05-19T14:30:32Z

💚 Build Succeeded

the below badges are clickable and redirect to their specific view in the CI or DOCS

Expand to view the summary

Build stats

Start Time: 2022-05-23T08:45:39.771+0000
Duration: 102 min 49 sec

Test stats 🧪

Test	Results
Failed	0
Passed	22293
Skipped	1933
Total	24226

💚 Flaky test report

Tests succeeded.

🤖 GitHub comments

To re-run your PR in the CI, just comment with:

/test : Re-trigger the build.
/package : Generate the packages and run the E2E tests.
/beats-tester : Run the installation tests with beats-tester.
run elasticsearch-ci/docs : Re-trigger the docs validation. (use unformatted text in the comment!)

elasticmachine · 2022-05-19T17:03:13Z

Pinging @elastic/elastic-agent-data-plane (Team:Elastic-Agent-Data-Plane)

andrewvc · 2022-05-19T20:18:29Z

x-pack/heartbeat/heartbeat.reference.yml

@@ -287,6 +287,10 @@ heartbeat.jobs:
 # sub-dictionary. Default is false.
 #fields_under_root: false

+# Configure the precision of all timestamps in Heartbeat.


Am I right in understanding that this basically just changes how time.Time fields are serialized when used with common.Time? If so, this should work.

However, we should add a note that time fields will keep their us (microsecond) paths, such as monitor.duration.us.

Nevermind, I think we should be fine here, all the us stuff is durations not timestamps.

andrewkroh

I assume (but not sure) this is meant to control the precision of timestamps used in events sent by outputs. We need to add a test case for this. I don't think changing the implementation of common.Time#MarshalJSON is sufficient.

The outputs that allow configurable formats have a concept of a codec that is involved in controlling the output format. Perhaps the configuration for this belongs in the codecs that are responsible for marshaling the data.

For example the outputs/codec/json already supports some configuration in the timestamp format by allowing the local time zone to be used.

beats/libbeat/outputs/codec/json/json.go

Lines 80 to 86 in 9b82b2a

    
           // create new encoder with custom time.Time encoding 
        
           e.folder, err = gotype.NewIterator(visitor, 
        
           	gotype.Folders( 
        
           		codec.MakeUTCOrLocalTimestampEncoder(e.config.LocalTime), 
        
           		codec.MakeBCTimestampEncoder(), 
        
           	), 
        
           )

And the format of the timestamps used by the Elasticsearch output are controlled here:

beats/libbeat/esleg/eslegclient/enc.go

Lines 90 to 93 in b581b17

    
           b.folder, err = gotype.NewIterator(visitor, 
        
           	gotype.Folders( 
        
           		codec.MakeTimestampEncoder(), 
        
           		codec.MakeBCTimestampEncoder()))

andrewkroh · 2022-05-19T22:22:57Z

libbeat/common/datetime.go

+	return nil
+}
+
+func SetTimestampPrecision(c *conf.C) error {


This seems important and I think it needs some documentation. Like it should state that it modifies the behavior of common.Time's MarshalJSON() and String() methods . And I think it should call out that it's not safe to call this after you have any other threads using common.Time because it modifies the global formatter that they all share.

andrewvc · 2022-05-20T01:06:18Z

Thinking about this more, I'm wondering if we can opt out of this for heartbeat. This isn't a feature any of our users want / need, and it only creates a support burden. If we can decrease heartbeat's surface area where possible that's a win.

kvch · 2022-05-20T06:34:04Z

@andrewkroh @andrewvc For the record the timestamps Beats produce are already in nanosecond precision as implemented in #31553

This PR is just making it configurable for users.

Thinking about this more, I'm wondering if we can opt out of this for heartbeat. This isn't a feature any of our users want / need, and it only creates a support burden. If we can decrease heartbeat's surface area where possible that's a win.

What you you mean by opt out? Do you want to opt out of configuring timestamp precision and stay with the new nanosecond setting? Or do you want to stay on the previous millisecond precision? If you want to opt out of nanosecond precision timestamps, this is the PR that implements it for you. :)

kvch · 2022-05-20T07:13:43Z

I assume (but not sure) this is meant to control the precision of timestamps used in events sent by outputs. We need to add a test case for this.

@andrewkroh Initially, it was my intention. However, only changing the timestamp precision of @timestamp of events is just a partial solution. If an input or dataset supports different precisions we should also make it available to users or integrations developers. It seems a bit weird that e.g. someone changes the timestamps in an Ingest pipeline and they loose the higher precision value.

My reasoning is that all timestamps were globally millisecond precision in Beats. The precisions of timestamps should be configured together where possible. It looks inconsistent if one timestamp is nanosecond, another one is in milliseconds, etc.

I don't think changing the implementation of common.Time#MarshalJSON is sufficient.

It's not only changing MarshalJSON, the PR also affects String function. Event transformers, serializers use the String function of common.Time to get the string representation of the field. (TBH I haven't seen anyone using MarshalJSON to marshal timestamp fields.)

I indeed missed MakeUTCOrLocalTimestampEncoder, I will pass down the configuration there as well. Thanks for finding it.

kvch · 2022-05-20T12:55:53Z

~~Note to self: check if datetime has to be updated in libs as well.~~ Yes...

andrewkroh

I would rather see the output format controlled without the use of a global state in a common package. The global nature makes test isolation difficult because one test can modify a global and affect other tests. This is less of a concern, but it also creates a concurrency issue since there is no synchronization on the global. Rather than using a global, the desired format could be passed into each output codec instance.

I have a similar feeling about the global nature of timestamp.precision. If this were scoped to the output config this might give a bit more flexibility in the behavior if it were ever needed (like allowing independent precision for the monitoring output vs the event output).

andrewkroh · 2022-05-23T11:53:16Z

libbeat/common/datetime.go

@@ -87,7 +171,7 @@ func ParseTime(timespec string) (Time, error) {
 }

 func (t Time) String() string {
-	str, _ := defaultTimeFormatter.Format(time.Time(t).UTC())
+	str, _ := timeFormatter.Format(time.Time(t).UTC())


It's not only changing MarshalJSON, the PR also affects String function. Event transformers, serializers use the String function of common.Time to get the string representation of the field.

Apart from some test code that uses the stdlib json encoder, I can't think of anything that should be dependent upon common.Time's format in the output path (including processors). Could common.Time retain its static behavior of generating ISO8601 timestamps with nanosecond precision without affecting the output format?

I'm not blocking the change.

kvch · 2022-05-23T13:31:06Z

I would rather see the output format controlled without the use of a global state in a common package. The global nature makes test isolation difficult because one test can modify a global and affect other tests. This is less of a concern, but it also creates a concurrency issue since there is no synchronization on the global. Rather than using a global, the desired format could be passed into each output codec instance.

I agree. My initial implementation included a new common.Time struct that included a precision, so each timestamp can have a separate precision. However, after refactoring the code for a long time, I reconsidered my plans because it required tremendous change. But your idea sounds simpler. I will merge this PR as is for now. I have to adopt the changes in elastic-agent-libs as well. There I will use your approach and then adopt it in beats. Thank you!

I have a similar feeling about the global nature of timestamp.precision. If this were scoped to the output config this might give a bit more flexibility in the behavior if it were ever needed (like allowing independent precision for the monitoring output vs the event output).

On the input side users can rely on the timestamp processor to modify the precision of the timestamp. With that in mind, I think we can get away with setting the granularity globally to nanoseconds, and then decrease it on the input level if needed.

configurable timestamp precision

f962b08

botelastic bot added the needs_team Indicates that the issue/PR needs a Team:* label label May 19, 2022

mergify bot assigned kvch May 19, 2022

check error

d64d992

kvch added 4 commits May 19, 2022 17:32

fix panic

6fc131f

fix issues reported by linter

ff09f78

more fixes

6072440

add docs

82d66a6

kvch force-pushed the nanosecond-opt-in-ii branch from 4d55be5 to 82d66a6 Compare May 19, 2022 17:01

kvch changed the title ~~configurable timestamp precision~~ New option timestamp.precision to control timestamp precision May 19, 2022

update docs

afd7dec

kvch marked this pull request as ready for review May 19, 2022 17:02

kvch requested review from a team as code owners May 19, 2022 17:02

kvch requested review from rdner and fearful-symmetry and removed request for a team May 19, 2022 17:02

kvch added the Team:Elastic-Agent-Data-Plane Label for the Agent Data Plane team label May 19, 2022

botelastic bot removed the needs_team Indicates that the issue/PR needs a Team:* label label May 19, 2022

cmacknz approved these changes May 19, 2022

View reviewed changes

andrewvc reviewed May 19, 2022

View reviewed changes

andrewkroh self-requested a review May 19, 2022 22:37

andrewkroh previously requested changes May 19, 2022

View reviewed changes

address review notes

826e911

kvch added 2 commits May 20, 2022 13:01

add more tests

a30f629

fix packetbeat tests

09b735a

kvch added 2 commits May 20, 2022 15:38

use nanosec precision in filebeat python tests

6db5bbc

more packetbeat tests

6808a65

kvch requested a review from andrewkroh May 20, 2022 13:59

use nanosecond precision in xpack modules tests

6683d70

andrewkroh reviewed May 23, 2022

View reviewed changes

kvch merged commit b63e5be into elastic:main May 23, 2022

chrisberkhout pushed a commit that referenced this pull request Jun 1, 2023

New option timestamp.precision to control timestamp precision (#31682)

f59cad1

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

New option `timestamp.precision` to control timestamp precision #31682

New option `timestamp.precision` to control timestamp precision #31682

kvch commented May 19, 2022 •

edited

Loading

elasticmachine commented May 19, 2022 •

edited by jenkins-beats-ci bot

Loading

Build stats

Test stats 🧪

elasticmachine commented May 19, 2022

andrewvc May 19, 2022 •

edited

Loading

andrewvc May 19, 2022

andrewkroh left a comment •

edited

Loading

andrewkroh May 19, 2022

andrewvc commented May 20, 2022

kvch commented May 20, 2022 •

edited

Loading

kvch commented May 20, 2022

kvch commented May 20, 2022 •

edited

Loading

andrewkroh left a comment

andrewkroh May 23, 2022

kvch commented May 23, 2022

	// create new encoder with custom time.Time encoding
	e.folder, err = gotype.NewIterator(visitor,
	gotype.Folders(
	codec.MakeUTCOrLocalTimestampEncoder(e.config.LocalTime),
	codec.MakeBCTimestampEncoder(),
	),
	)

	b.folder, err = gotype.NewIterator(visitor,
	gotype.Folders(
	codec.MakeTimestampEncoder(),
	codec.MakeBCTimestampEncoder()))

New option timestamp.precision to control timestamp precision #31682

New option timestamp.precision to control timestamp precision #31682

Conversation

kvch commented May 19, 2022 • edited Loading

What does this PR do?

Why is it important?

Checklist

elasticmachine commented May 19, 2022 • edited by jenkins-beats-ci bot Loading

💚 Build Succeeded

Build stats

Test stats 🧪

💚 Flaky test report

🤖 GitHub comments

elasticmachine commented May 19, 2022

andrewvc May 19, 2022 • edited Loading

Choose a reason for hiding this comment

andrewvc May 19, 2022

Choose a reason for hiding this comment

andrewkroh left a comment • edited Loading

Choose a reason for hiding this comment

andrewkroh May 19, 2022

Choose a reason for hiding this comment

andrewvc commented May 20, 2022

kvch commented May 20, 2022 • edited Loading

kvch commented May 20, 2022

kvch commented May 20, 2022 • edited Loading

andrewkroh left a comment

Choose a reason for hiding this comment

andrewkroh May 23, 2022

Choose a reason for hiding this comment

kvch commented May 23, 2022

New option `timestamp.precision` to control timestamp precision #31682

New option `timestamp.precision` to control timestamp precision #31682

kvch commented May 19, 2022 •

edited

Loading

elasticmachine commented May 19, 2022 •

edited by jenkins-beats-ci bot

Loading

andrewvc May 19, 2022 •

edited

Loading

andrewkroh left a comment •

edited

Loading

kvch commented May 20, 2022 •

edited

Loading

kvch commented May 20, 2022 •

edited

Loading