[v2] accept unix epoch timestamps in microseconds #1413

roncohen · 2018-10-01T21:32:38Z

for intake api v2, "timestamp" now accept only unix epoch timestamps in microseconds
makes span "start" optional if span "timestamp" is given for v2
if "timestamp" is not given, it is set to the sum of req-time + start for v2
adds timestamp.us (long) to span, transaction and error documents for v2 only

roncohen · 2018-10-01T21:34:48Z

_meta/fields.common.yml

+      fields:
+        - name: us
+          type: long
+          count: 1


I wasn't able to figure out what this "count: 1" actually does.

* makes "start" optional if "timestamp" is given for v2 * if "timestamp" is not given, it is set to the sum of req-time + start * adds timestamp.us (long) to span, transaction and error documents (both v1 and v2)

roncohen · 2018-10-02T08:56:06Z

after a discussion with @simitt and @sqren I'm going to remove the new timestamp.us from v1 data.

roncohen · 2018-10-02T15:00:58Z

I've made the changes necessary to keep timestamp.us out of v1 data in c988dc9.

It adds significant complexity, in particular in the tests. Given the added complexity, i think it's worth re-considering if we shouldn't "just" add timestamp.us also to the v1 data.

The UI will probably not be able to take advantage of this added field for v1 data, because the UI needs to take into account old data that doesn't have the field anyway - so it's easier for it to just assume there no timestamp.us field for v1 data.

sorenlouv · 2018-10-02T15:07:48Z

If this is not worth it, I'm fine with leaving timestamp.us in, and the UI will just not use it when handling v1 data.

docs/spec/metricsets/v1_metricset.json

docs/spec/metricsets/v2_metricset.json

graphaelli · 2018-10-02T20:35:31Z

docs/spec/metricsets/v2_metricset.json

+            "properties": {
+
+                "timestamp": {
+                    "description": "Recorded time of the span, UTC based and formatted as microseconds since Unix epoch",


What do you think of adding something like timestamp_datetime.json timestamp_epoch.json and $ref to those to make it clear where the differences are?

graphaelli · 2018-10-02T20:37:21Z

model/error/event.go

 	e.TransactionId = decoder.StringPtr(raw, "id", "transaction")
-	return e, decoder.Err
+	if decoder.Err != nil {


why change this?

It started with the fact that we have this:
https://github.com/elastic/apm-server/pull/1413/files#diff-fcd6e0b6a82640c441aa919ee2076b33L199

which means that if there's an error, V1DecodeEvent must also return nil, err. As it turns out, previously, we never tested the path in which the decoder would set decoder.Err here. Modifying the snippet below (from v2 branch) would still pass all tests:

func V1DecodeEvent(input interface{}, err error) (transform.Transformable, error) { e, raw, err := decodeEvent(input, err) if err != nil { return nil, err } decoder := utility.ManualDecoder{} e.TransactionId = decoder.StringPtr(raw, "id", "transaction") + if decoder.Err != nil { + panic(decoder.Err) + } return e, decoder.Err }

I'll make sure it gets tested and also simplify the tests to not care about the first return argument if there's an error returned.

to be clear: my first approach was to just align it across the model and forcing the nil return on decoder errors bring it in line with the others. I'll simplify across the board now instead.

graphaelli · 2018-10-03T00:59:22Z

model/error/event.go

+func (e *v2Event) Transform(tctx *transform.Context) []beat.Event {
+	events := e.Event.Transform(tctx)
+	for _, e := range events {
+		utility.Add(e.Fields, "timestamp", utility.TimeAsMicros(e.Timestamp))


This isn't sitting well with me, though I see how we got here. Have you considered adding a member to Event to differentiate between v1 and v2 and only add this timestamp object if v2, inside of Event.Transform()?

much better, thanks!

Have you considered creating two different event attributes for the two timestamps? This would allow setting the proper timestamp in the v1/v2 decoding methods. The transformation method could then only transform what is set, without any additional knowledge.

The reason I am suggesting this is that we could keep the whole v1/v2 separation in the decode method, as we do so far, which is also the reason that so far no separate v2 transform tests have been necessary.

I think I see your point. If i understand correctly, you're suggesting to do something like:

type Event struct { Name string Type string Start *float64 Duration float64 Context common.MapStr Stacktrace m.Stacktrace - Timestamp time.Time TransactionId string // new in v2 HexId string ParentId string TraceId string + TimestampV2 time.Time // deprecated in v2 Id *int64 Parent *int64 + TimestampV1 time.Time }

func V1DecodeEvent(input interface{}, err error) (transform.Transformable, error) { ... e.TimestampV1 = decoder.TimeRFC3339(raw, "timestamp") ... } func V2DecodeEvent(input interface{}, err error) (transform.Transformable, error) { ... e.TimestampV2 = decoder.TimeEpochMicro(raw, "timestamp") ... }

For spans, it seems to me that the transform method needs to know if it is dealing with a v1 span or a v2 span. Part of the new functionality is to set the timestamp to be reqTime + start if no timestamp is given. For v1 we're just setting it to reqTime. Looking at the span transform method, if none of the timestamps are set (timestamp is optional in both v1 and v2) how will we know if we should apply the v2 logic or not?

Besides the problem above, what i like about this current solution is that the decode methods "decode" to a model that is shared by v1 and v2 in the sense there's one field to denote the timestamp, regardless of which format it came in.

Having two timestamp fields for the sake of deciding the final output seems less clean to me, compared to decoding the different input representations with the same semantics into the same field as it is currently. For example, if there was some functionality that required us to use the timestamp in some logic, if we have two timestamps we would need to check them both even though the semantically mean the same thing.

Appreciate the explanation, agree that it is cleaner this way for spans.

roncohen · 2018-10-03T13:59:53Z

model/metricset/event_test.go

@@ -130,6 +131,19 @@ func TestDecode(t *testing.T) {
 		}
 	}
 }
+func TestDecodeV1(t *testing.T) {


this parameterizes testDecode while in "span" the decode tests are simply copied. I'm fine either way, but we should consider aligning them.

The span and transaction events have quite a few differences, whereas the metricset only differs in the time. Therefore, for spans I specifically separated v1 and v2 tests, except for the failures, that were expected to be the same for both.

So you're OK with the current changes?

yes fine with me

roncohen · 2018-10-03T14:08:28Z

following feedback from @graphaelli things are a bit simpler and I'm OK to move forward with the current functionality.

graphaelli

lgtm. Note that model/transaction/event_test.go'sTestEventTransform doesn't cover v2 events specifically - I don't think that needs to hold this up as integration tests do cover it for now, we should at least revisit using coverage tools later on.

simitt · 2018-10-05T08:16:09Z

docs/spec/timestamp_epoch.json

+    "type": ["object"],
+    "properties": {  
+        "timestamp": {
+            "description": "Recorded time of the span, UTC based and formatted as microseconds since Unix epoch",


This should say event rather than span.

simitt · 2018-10-05T08:57:38Z

model/metricset/event_test.go

@@ -130,6 +131,19 @@ func TestDecode(t *testing.T) {
 		}
 	}
 }
+func TestDecodeV1(t *testing.T) {


The span and transaction events have quite a few differences, whereas the metricset only differs in the time. Therefore, for spans I specifically separated v1 and v2 tests, except for the failures, that were expected to be the same for both.

roncohen · 2018-10-07T23:21:01Z

thanks @graphaelli and @simitt !

roncohen · 2018-10-07T23:23:27Z

@graphaelli i added your comment as a todo here: #1237

* makes "start" optional if "timestamp" is given for v2 * if "timestamp" is not given, it is set to the sum of req-time + start if data arrives through v2 * adds `timestamp.us` (long) to span, transaction and error documents for v2

felixbarny · 2018-10-08T07:37:36Z

Note for agent devs: the field which holds the epoch micros is still called timestamp in the intake API: https://github.com/elastic/apm-server/blob/v2/docs/spec/timestamp_epoch.json

see elastic/apm-server#1413

* makes "start" optional if "timestamp" is given for v2 * if "timestamp" is not given, it is set to the sum of req-time + start if data arrives through v2 * adds `timestamp.us` (long) to span, transaction and error documents for v2

roncohen requested review from simitt and graphaelli October 1, 2018 21:32

roncohen commented Oct 1, 2018

View reviewed changes

[v2] accept unix epoch timestamps in microseconds

126e9b3

* makes "start" optional if "timestamp" is given for v2 * if "timestamp" is not given, it is set to the sum of req-time + start * adds timestamp.us (long) to span, transaction and error documents (both v1 and v2)

roncohen force-pushed the v2-timestamps branch from 745689a to 126e9b3 Compare October 1, 2018 21:43

Do not add timestamp.us in v1.

c988dc9

alvarolobato added the [zube]: In Progress label Oct 2, 2018

graphaelli reviewed Oct 2, 2018

View reviewed changes

graphaelli reviewed Oct 3, 2018

View reviewed changes

fixes

d90b4d6

roncohen commented Oct 3, 2018

View reviewed changes

Ron cohen added 2 commits October 3, 2018 16:02

update test data to test case with start and timestamp

46d69e3

updated for v2_metricset.json

1f431dd

forgot to update the inline schemas 🤦

827bf76

graphaelli approved these changes Oct 3, 2018

View reviewed changes

graphaelli assigned roncohen Oct 5, 2018

simitt reviewed Oct 5, 2018

View reviewed changes

update timestamp_epoch description

6fc18e1

simitt approved these changes Oct 6, 2018

View reviewed changes

updated schemas

ac2c073

roncohen merged commit 545bfba into elastic:v2 Oct 7, 2018

roncohen deleted the v2-timestamps branch October 7, 2018 23:20

zube bot added [zube]: Done and removed [zube]: In Progress labels Oct 7, 2018

roncohen mentioned this pull request Oct 7, 2018

Intake v2 support #1237

Closed

30 tasks

roncohen mentioned this pull request Oct 7, 2018

v2: Require either span.start or span.timestamp #1340

Closed

8 tasks

felixbarny added a commit to felixbarny/apm-agent-java that referenced this pull request Oct 8, 2018

Use epoch micros for intake v2

839bb41

see elastic/apm-server#1413

felixbarny mentioned this pull request Oct 8, 2018

Use epoch micros for intake v2 elastic/apm-agent-java#240

Merged

felixbarny added a commit to felixbarny/apm-agent-java that referenced this pull request Oct 8, 2018

Use epoch micros for intake v2

f28a2ef

see elastic/apm-server#1413

felixbarny added a commit to elastic/apm-agent-java that referenced this pull request Oct 15, 2018

Use epoch micros for intake v2 (#240)

e29f1e2

see elastic/apm-server#1413

felixbarny mentioned this pull request Oct 15, 2018

Allow timestamps to be in epoch ms #850

Closed

simitt mentioned this pull request Oct 16, 2018

Possible data issues due to timestamp being optional. #1314

Closed

YuZhenXie mentioned this pull request Aug 9, 2019

Upgrading the API to support APM server v2 philkra/elastic-apm-php-agent#82

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[v2] accept unix epoch timestamps in microseconds #1413

[v2] accept unix epoch timestamps in microseconds #1413

roncohen commented Oct 1, 2018 •

edited by jalvz

Loading

roncohen Oct 1, 2018

roncohen commented Oct 2, 2018

roncohen commented Oct 2, 2018

sorenlouv commented Oct 2, 2018 •

edited

Loading

graphaelli Oct 2, 2018

graphaelli Oct 2, 2018

roncohen Oct 3, 2018

roncohen Oct 3, 2018

graphaelli Oct 3, 2018

roncohen Oct 3, 2018

simitt Oct 5, 2018

roncohen Oct 5, 2018

simitt Oct 6, 2018

roncohen Oct 3, 2018 •

edited

Loading

simitt Oct 5, 2018

roncohen Oct 5, 2018

simitt Oct 6, 2018

roncohen commented Oct 3, 2018

graphaelli left a comment

simitt Oct 5, 2018

simitt Oct 5, 2018

roncohen commented Oct 7, 2018

roncohen commented Oct 7, 2018

felixbarny commented Oct 8, 2018

[v2] accept unix epoch timestamps in microseconds #1413

[v2] accept unix epoch timestamps in microseconds #1413

Conversation

roncohen commented Oct 1, 2018 • edited by jalvz Loading

Choose a reason for hiding this comment

roncohen commented Oct 2, 2018

roncohen commented Oct 2, 2018

sorenlouv commented Oct 2, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

roncohen Oct 3, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

roncohen commented Oct 3, 2018

graphaelli left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

roncohen commented Oct 7, 2018

roncohen commented Oct 7, 2018

felixbarny commented Oct 8, 2018

roncohen commented Oct 1, 2018 •

edited by jalvz

Loading

sorenlouv commented Oct 2, 2018 •

edited

Loading

roncohen Oct 3, 2018 •

edited

Loading