Skip to content
This repository has been archived by the owner on Aug 14, 2024. It is now read-only.

feat(insights): asset module documentation #1306

Merged
merged 13 commits into from
Jun 13, 2024

Conversation

DominikB2014
Copy link
Contributor

Initial asset module docs

@DominikB2014 DominikB2014 requested a review from gggritso June 10, 2024 19:08
Copy link

vercel bot commented Jun 10, 2024

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

Name Status Preview Comments Updated (UTC)
develop ✅ Ready (Inspect) Visit Preview 💬 Add feedback Jun 13, 2024 5:14pm

Copy link
Member

@gggritso gggritso left a comment

Choose a reason for hiding this comment

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

Right on! A bunch of nits only, overall makes sense 👍🏻

src/docs/sdk/performance/modules/assets.mdx Outdated Show resolved Hide resolved
src/docs/sdk/performance/modules/assets.mdx Outdated Show resolved Hide resolved
src/docs/sdk/performance/modules/assets.mdx Outdated Show resolved Hide resolved

The description of an asset span should be the full URL which is was fetched from, e.g. `https://cdn.com/main.hash123.js`.

### Span Data
Copy link
Member

Choose a reason for hiding this comment

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

This section should link to the "Span Data Conventions" section for resources. That way you don't have to re-do the table. You just need to explain which of those attribute are mandatory (all of them?)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

🤔 There is no "resource" section in span data conventions. Resource spans contains some properties from the browser section and some properties from the http section

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Also, what's the meaning of required here, technically the UI will work with only encoded body size passed in, although not all data will be populated in the UI.

Copy link
Member

Choose a reason for hiding this comment

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

Ah, I think you should add a section, then, or at least somehow clarify what's going on. As for "meaning of required" that's the question, because it's this document's job to explain that, in detail! e.g., in "Requests" I explain which span attribute is required for which part of the UI. I think you can do the same here!

src/docs/sdk/performance/modules/assets.mdx Outdated Show resolved Hide resolved
src/docs/sdk/performance/modules/assets.mdx Outdated Show resolved Hide resolved
src/docs/sdk/performance/modules/assets.mdx Outdated Show resolved Hide resolved
DominikB2014 and others added 4 commits June 11, 2024 10:32
Co-authored-by: George Gritsouk <[email protected]>
Co-authored-by: George Gritsouk <[email protected]>
Co-authored-by: George Gritsouk <[email protected]>
Co-authored-by: George Gritsouk <[email protected]>
@DominikB2014 DominikB2014 merged commit 77245ad into master Jun 13, 2024
5 checks passed
@DominikB2014 DominikB2014 deleted the DominikB2014/asset-module-docs branch June 13, 2024 17:42
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants