feat(apm) Expose transaction start & end timestamp #14007

markstory · 2019-07-15T17:15:10Z

On transaction events expose both the start and end timestamps. This will allow us to correctly display the transaction span length.

I've also added the transaction sample from my now closed tracing pull request. Having a sample transaction event should be useful when we have to do more endpoint and acceptance tests around transaction events.

Refs SEN-799

fpacifici · 2019-07-15T23:59:17Z

src/sentry/api/serializers/models/event.py

+        # timestamp floats.
+        if obj.get_event_type() == 'transaction':
+            d['startTimestamp'] = obj.data.get('start_timestamp')
+            d['timestamp'] = obj.data.get('timestamp')


I think this ends up being the same as obj.datetime and the field dateCreated that is populated at line 272. Is there a reason to add a new one ?

The dateCreated field is a string and lacks the ms precision.

then, since this is going to be there only for transacitons, I would advise to rename the two fields in such a way that make it clear what they mean like:
startTimestamp
endTimestamp

So people would not have to look into the code to understand what timestamp means in the context of the transaction.

dashed · 2019-07-16T00:59:06Z

Is it possible to have a dedicated transaction object?

Something like

{
    // ...
    
    "transaction": {
        "name": "/country_by_code/",
        "start_timestamp": 1562681591.0,
        "end_timestamp": 1562681591.0,
    },

    // left alone for other parts of the frontend
    "timestamp": 1562681591.0,
}

markstory · 2019-07-16T15:19:56Z

@dashed I'm reluctant to overload transaction as it means something different in the application internals. I could however add the name, start & end timestamps to metadata as that is a flex slot that varies by event type.

dashed · 2019-07-16T16:09:56Z

@markstory Yea the metadata slot may be suitable. As long as the transaction attributes are grouped together somewhere 😄 .

I feel like attributes start_timestamp don't really belong at the top level.

markstory · 2019-07-16T17:40:36Z

I've moved the start & end timestamps into the TransactionEvent metadata value. This resolves the structural issues in the API response, but has an impact on storage.

With timestamps in the metadata, the generated message that ends up in clickhouse contains both timestamps. An example of this is 1563298402.26 1563298402.24 /cities/

dashed · 2019-07-16T19:41:46Z

I don't think we want the message to be like that 😬

markstory · 2019-07-17T19:11:13Z

@dashed I've done some refactoring in EventManager to move the message generation into the event types. This change will prevent the timestamps from being indexed in the message.

@fpacifici Do you think the timestamp names need to change if they are located in metadata? I'd like to avoid having different names between the transaction event and the spans if possible.

src/sentry/eventtypes/transaction.py

dashed · 2019-07-17T19:50:16Z

This looks pretty good to me. 👏

I'll leave the implementation details to @fpacifici since I don't have strong opinions on that front.

fpacifici · 2019-07-23T17:02:23Z

Do you think the timestamp names need to change if they are located in metadata? I'd like to avoid having different names between the transaction event and the spans if possible.

meh .... ok. If we are end up being inconsistent either way feel free to ignore my comment.

src/sentry/eventtypes/base.py

fpacifici · 2019-07-23T17:25:46Z

src/sentry/utils/samples.py


    data = CanonicalKeyDict(data)
-    if platform in ('csp', 'hkpk', 'expectct', 'expectstaple'):
+    if platform in ('transaction', 'csp', 'hkpk', 'expectct', 'expectstaple'):


Why is this change needed ? It was not here in the previous version if I remember correctly

Without this the sample transaction event gets data added to the extra, modules and request interfaces which are generally not going to be present on a transaction event payload.

Ok, but "transaction" should not be a platform. By doing this we are overloading the meaning and behavior of a parameter. Also you are skipping line 145 where the "platform" field is added to data. Is this intentional.
If you need a different behavior for different event types, I would suggest to pass the event type as an optional parameter, or, better pass a parameter that tells which interfaces to pass.

The variable name platform is an awkward name as it is used to locate a sample/fixture file in src/sentry/data/samples. The files there are a mixture of both sample SDK events and special scenarios like empty-exception or python-omittedbody.

On transaction events expose both the start and end timestamps. This will allow us to correctly display the transaction span length. Refs SEN-799

Move logic to serialize transaction only events into a separate method. This also allows error/default only logic to be moved as well. I've elected to not emit `dateCreated` on transaction events as it is redundant given the start/end timestamps.

markstory · 2019-07-24T17:15:04Z

I've removed the metadata attribute. I talked with @jan-auer and event metadata is intended to capture data that needs to be persisted to the 'group'. Since transaction events don't have groups using this doesn't follow the original intent.

I've gone back to exposing timestamps on the root level of the event and using startTimestamp and endTimestamp to be more clear. While this creates some dissimilarity with how spans represent ending timestamp, I'm open to renaming those properties as well.

I've also shifted some of the properties that will only appear on error events around so that those keys are never populated on transaction events.

dashed

Looks good to me. 👍

I'll let either @fpacifici or Vienna give their feedback (cc @jan-auer )

src/sentry/api/serializers/models/event.py

fpacifici · 2019-07-24T17:43:21Z

src/sentry/api/serializers/models/event.py

            'metadata': obj.get_event_metadata(),
            'tags': tags,
            'platform': obj.platform,
-            'dateCreated': obj.datetime,


I noticed a few dependencies on dateCreated

src/sentry/static/sentry/app/views/organizationEventsV2/eventModalContent.jsx: date={getDynamicText({value: event.dateCreated, fixed: 'Dummy timestamp'})}
src/sentry/static/sentry/app/views/organizationEventsV2/modalLineGraph.jsx: const eventTime = +new Date(currentEvent.dateCreated);
src/sentry/static/sentry/app/views/organizationGroupDetails/eventToolbar.jsx: const dateCreated = moment(evt.dateCreated);

Are these going to be ok if you remove the field? I suspect there is no acceptance test that go through transaction event.

Good finds. The modal graph and content component do need to be updated. The GroupDetailsToolbar should be fine as transaction events won't have groups.

fpacifici · 2019-07-24T21:22:13Z

I am ok with this now. @dashed you may want to have a look at the UI change.

markstory requested review from a team and fpacifici July 15, 2019 17:15

fpacifici reviewed Jul 15, 2019

View reviewed changes

dashed mentioned this pull request Jul 16, 2019

feat(apm): Initial transactions view #13920

Merged

36 tasks

markstory force-pushed the sen-799-start-timestamp branch from 913a769 to 61522bd Compare July 16, 2019 17:38

markstory force-pushed the sen-799-start-timestamp branch from 61522bd to e1e8cb6 Compare July 17, 2019 19:12

dashed reviewed Jul 17, 2019

View reviewed changes

src/sentry/eventtypes/transaction.py Outdated Show resolved Hide resolved

markstory requested a review from fpacifici July 22, 2019 13:46

markstory mentioned this pull request Jul 22, 2019

feat(eventsv2) Add basic transaction list #14103

Merged

fpacifici reviewed Jul 23, 2019

View reviewed changes

markstory added 2 commits July 24, 2019 11:43

feat(apm) Expose transaction start & end timestamp

e26e352

On transaction events expose both the start and end timestamps. This will allow us to correctly display the transaction span length. Refs SEN-799

markstory force-pushed the sen-799-start-timestamp branch from eb1cb2f to 0c40d72 Compare July 24, 2019 17:12

dashed approved these changes Jul 24, 2019

View reviewed changes

fpacifici reviewed Jul 24, 2019

View reviewed changes

markstory added 2 commits July 24, 2019 14:56

Use dateCreated string or endTimestamp for transaction events.

bc6b388

Make event type serialization methods private

1344a2d

markstory merged commit b5a0a68 into master Jul 25, 2019

joshuarli deleted the sen-799-start-timestamp branch August 27, 2019 20:11

github-actions bot locked and limited conversation to collaborators Dec 20, 2020

Uh oh!

feat(apm) Expose transaction start & end timestamp #14007

feat(apm) Expose transaction start & end timestamp #14007

Uh oh!

Conversation

markstory commented Jul 15, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dashed commented Jul 16, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

markstory commented Jul 16, 2019

Uh oh!

dashed commented Jul 16, 2019

Uh oh!

markstory commented Jul 16, 2019

Uh oh!

dashed commented Jul 16, 2019

Uh oh!

markstory commented Jul 17, 2019

Uh oh!

Uh oh!

dashed commented Jul 17, 2019

Uh oh!

fpacifici commented Jul 23, 2019

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

markstory commented Jul 24, 2019

Uh oh!

dashed left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fpacifici commented Jul 24, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

dashed commented Jul 16, 2019 •

edited

Loading