Skip to content

Conversation

hezhizhen
Copy link
Contributor

Summary

  1. Add ctx parameter for most Devbox methods
  2. Add trace tasks for Devbox methods
  3. Add regions for sub-methods

How was it tested?

@hezhizhen
Copy link
Contributor Author

The PR is still working in progress. Is it a good idea to trace them all? @savil @mikeland86

@hezhizhen hezhizhen changed the title [trace] Add trace task for Devbox methods [trace] Add trace for Devbox methods Jun 3, 2023
@savil
Copy link
Collaborator

savil commented Jun 5, 2023

@gcurtis do you have thoughts on this? I know you'd added the initial tracing infra, and dashboards.

My concern would be that adding to each method may get overwhelming to track in the dashboards, but i'm not sure where to look for them.

@hezhizhen
Copy link
Contributor Author

@gcurtis do you have thoughts on this? I know you'd added the initial tracing infra, and dashboards.

My concern would be that adding to each method may get overwhelming to track in the dashboards, but i'm not sure where to look for them.

Any updates? @savil @gcurtis

@savil savil requested a review from gcurtis June 8, 2023 16:39
Copy link
Collaborator

@gcurtis gcurtis left a comment

Choose a reason for hiding this comment

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

This looks good to me. There aren't any dashboards for the CLI, it's just for devbox -trace which generates a go tool trace file to help track down performance issues.

@hezhizhen hezhizhen force-pushed the trace branch 3 times, most recently from ae475fa to 8682745 Compare June 11, 2023 08:22
@hezhizhen hezhizhen marked this pull request as ready for review June 11, 2023 08:22
@hezhizhen
Copy link
Contributor Author

The rest methods are so simple that I don't think it's necessary to add trace. PTAL @gcurtis

@hezhizhen hezhizhen force-pushed the trace branch 5 times, most recently from 457eec2 to 9511ec1 Compare June 11, 2023 12:41
@savil savil requested a review from gcurtis June 12, 2023 17:14
@hezhizhen
Copy link
Contributor Author

@gcurtis @savil Any comments?

Copy link
Collaborator

@gcurtis gcurtis left a comment

Choose a reason for hiding this comment

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

Thanks for cleaning all this up!

Comment on lines 4 to 6
// Copyright 2023 Jetpack Technologies Inc and contributors. All rights reserved.
// Use of this source code is governed by the license in the LICENSE file.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Duplicate?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah. That's weird. I removed it.

@savil
Copy link
Collaborator

savil commented Jun 15, 2023

@hezhizhen thanks! if you could resolve the conflcts, I can help merge.

@hezhizhen
Copy link
Contributor Author

@hezhizhen thanks! if you could resolve the conflcts, I can help merge.

Done. PTAL @savil

@savil savil merged commit 2870425 into jetify-com:main Jun 16, 2023
@hezhizhen hezhizhen deleted the trace branch August 9, 2023 14:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants