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

DEVPROD-5303: Track sectioning analytics #328

Merged
merged 41 commits into from
Aug 27, 2024

Conversation

SupaJoon
Copy link
Contributor

@SupaJoon SupaJoon commented Aug 21, 2024

DEVPROD-5303

Description

These code changes are based off of #325
Allow tracking section toggling with insights on command vs function, nested vs unnested and section pass/fail status when applicable.
Also allow tracking if the log loaded with jump to failing line and sectioning.

Have you have updated the analytics documentation if necessary?
The analytics doc has been updated.

@SupaJoon SupaJoon requested a review from a team as a code owner August 21, 2024 21:26
Copy link
Contributor

@minnakt minnakt left a comment

Choose a reason for hiding this comment

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

analytics lgtm

Copy link
Contributor

Choose a reason for hiding this comment

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

seems like this file got added accidentally (maybe add to .gitignore)?

open: !open,
"section.name": commandName,
"section.type": "command",
status,
Copy link
Contributor

Choose a reason for hiding this comment

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

I think the attributes should reference their parent object (otherwise we could ask, is it a task status? is it a build status? some other evergreen status?)

Suggested change
status,
"section.nested": !isTopLevelCommand,
"section.open": !open,
"section.status": status,

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I used "section.name" instead of "name" because "name" was already taken. The docs mention that namespaces should be primiarily be used to avoid name collisions and not indicate hierarchies. The docs aren't really clear what is considered a naming collision though. Your example does kind of highlight a naming collision between status types.

Comment on lines 40 to 44
nested: false,
open: !open,
"section.name": functionName,
"section.type": "function",
status,
Copy link
Contributor

Choose a reason for hiding this comment

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

same comment

sendEvent({
"function.name": functionName,
name: "Clicked open subsections button",
status,
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
status,
"function.status": status,

sendEvent({
"function.name": functionName,
name: "Clicked close subsections button",
status,
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
status,
"function.status": status,

Comment on lines 62 to 64
"jump.to.failing.line.enabled": settings.jumpToFailingLineEnabled,
name: "Viewed log with sections and jump to failing line",
"sections.enabled": settings.sectionsEnabled,
Copy link
Contributor

Choose a reason for hiding this comment

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

possibly want to reference settings as a parent object:

Suggested change
"jump.to.failing.line.enabled": settings.jumpToFailingLineEnabled,
name: "Viewed log with sections and jump to failing line",
"sections.enabled": settings.sectionsEnabled,
"settings.jump.to.failing.line.enabled": settings.jumpToFailingLineEnabled,
name: "Viewed log with sections and jump to failing line",
"settings.sections.enabled": settings.sectionsEnabled,

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think it's okay to not indicate hierarchy here because the names are unique

Copy link
Contributor

Choose a reason for hiding this comment

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

I actually agree with Minna here. I'm planning on reviewing all of our span attributes in our projects and adding this hierarchy. So that it better falls in line with otel standards and avoids excessive dot notation.

settings.jump_to_failing_line.enabled
settings.sections.enabled

I'm planning on doing this in a follow up pr but I just wanted to shed some light on my intentions.

Copy link
Contributor

Choose a reason for hiding this comment

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

ah that's a good callout, jump.to.failing.line probably creates excessive namespaces that we would never use otherwise (jump, jump.to, jump.to.failing)

@SupaJoon SupaJoon requested a review from khelif96 August 27, 2024 14:54
@@ -6,6 +6,7 @@ cypress/screenshots/
/src/**/*.css
build.txt
build.txt-e
vite.config.ts.timestamp-*
Copy link
Contributor

Choose a reason for hiding this comment

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

What is this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

it's from a vite bug: vitejs/vite#13267

@SupaJoon SupaJoon merged commit b00200d into evergreen-ci:main Aug 27, 2024
3 checks passed
@SupaJoon SupaJoon deleted the DEVPROD-5303 branch August 27, 2024 15:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants