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

add ui.ttid to span operations #776

Merged
merged 3 commits into from
Dec 13, 2022
Merged

add ui.ttid to span operations #776

merged 3 commits into from
Dec 13, 2022

Conversation

stefanosiano
Copy link
Member

We are going to add a ttid (time to initial display) span to the automatic activity transactions.
The use case is mainly for mobile developers.
The current activity transaction duration is not reliable to have an understanding of how much time it took to actually load an activity, due to possible other spans added during the transaction.

@vercel
Copy link

vercel bot commented Dec 6, 2022

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Updated
develop ✅ Ready (Inspect) Visit Preview Dec 12, 2022 at 7:01PM (UTC)

@stefanosiano
Copy link
Member Author

@AbhiPrasad @marandaneto @philipphofmann
Android's implementation is in this pr
Is there anything to consider to add this new span?

@@ -181,6 +181,7 @@ Serverless related spans are expected to follow OpenTelemetry's [Function as a S
| | app.start.cold | |
| ui | | An operation on a mobile UI |
| | ui.load | |
| | ui.ttid | |
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should that be part of app.start? or every screen would have its own ttid?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

every screen it's going to have its own ttid
we'll also add the ttfd (time to full display) to every screen, too, but that's for another pr

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should mention what ttid stands for = time to initial display. Maybe we could also call it
ui.load.inital ui.load.full? Most developers won't understand what ttid stands for and they have to go to our docs.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

could be even ui.load.initial_draw and ui.load.full_draw. But I agree ttid is probably not well-known, so I'd also vote for changing it

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That would be semantically wrong I guess, initial_draw would mean it's drawn but that refers only to the very first frame only.
full_draw would mean it's fully drawn, but it's not only about drawing but rather waiting for async content and redrawing it.
so maybe initial_display and full_dislay? it's pretty much the wording for both acronyms

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

so let's go with ui.load.initial_display and ui.load.full_display? These make sense to me

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@philipphofmann does it make sense to you on ios?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, that makes sense. I'm happier now with this name as it's self-explanatory.

Copy link
Member

@AbhiPrasad AbhiPrasad left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the operation makes sense to me

@philipphofmann
Copy link
Member

@stefanosiano, please also add ui.load.full_display.

@stefanosiano
Copy link
Member Author

@stefanosiano, please also add ui.load.full_display.

In this same pr? Shouldn't we wait after implementing it?
Or are we going to keep this pr unmerged until the sdk implement it?

@philipphofmann
Copy link
Member

I think it's OK to merge it already, even if we don't have the implementation. We have already decided on the name. We can always change the docs again.

Copy link
Member

@philipphofmann philipphofmann left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Copy link
Member

@philipphofmann philipphofmann left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants