Skip to content

Expand Azure register warning context#5000

Merged
jwhitlock merged 3 commits intotaskcluster:mainfrom
jwhitlock:wm-azure-logs
Dec 16, 2021
Merged

Expand Azure register warning context#5000
jwhitlock merged 3 commits intotaskcluster:mainfrom
jwhitlock:wm-azure-logs

Conversation

@jwhitlock
Copy link
Copy Markdown
Contributor

Add workerPoolId, providerId, and workerId to registration-error-warning context, logged from the Azure provider's register() function in worker-manager.

Add workerState to the warning context when the state is not REQUESTED, to help diagnose registration with an unexpected state.

Github Bug/Issue: #4999 (helps diagnose, doesn't yet fix)

@jwhitlock jwhitlock requested a review from a team as a code owner October 7, 2021 19:16
@jwhitlock jwhitlock requested review from leplatrem and petemoore and removed request for a team October 7, 2021 19:16
Copy link
Copy Markdown
Collaborator

@djmitche djmitche left a comment

Choose a reason for hiding this comment

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

Changes look good to me otherwise, but I don't know if you still want my review :)

Add workerPoolId, providerId, and workerId to registration-error-warning
context, logged from the Azure provider's register() function in
worker-manager.

Add workerState to the warning context when the state is not REQUESTED,
to help diagnose registration with an unexpected state.
Switch to patch level, use present tense.
@jwhitlock
Copy link
Copy Markdown
Contributor Author

I added a yarn generate commit, needed because I changed the spec for registrationErrorWarning.

The remaining failure of service-secrets is a intermittent network error when installing the yarn packages. This step passed on the last round, so I think this can be ignored.

Copy link
Copy Markdown
Member

@petemoore petemoore left a comment

Choose a reason for hiding this comment

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

Looks great, thanks John!

@jwhitlock
Copy link
Copy Markdown
Contributor Author

Every time I think about merging this, I look at the repeated code and think there must be a way to get those variables automatically into the logging context.

I'm still holding off until 1) I can research that, or 2) we cut a new TC version

workerPoolId: workerPool.workerPoolId,
providerId: this.providerId,
workerId: worker.workerId,
});
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Inside this method (so, inside the scope wehre workerPool, this, and worker are defined), you could define a closure to do this:

   let logRegError = (err, message) => {
      this.monitor.log.registrationErrorWarning({
        message, err,
        workerPoolId: workerPool.workerPoolId,
        providerId: this.providerId,
        workerId: worker.workerId,
      });

Another option is to make a child logger that contains some of those values, but then you're passing a reference to that logger everywhere.

TBH I wouldn't worry about the verbosity here.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

option 3 - Add a comment and Dustin will prototype the solution 😁

Thanks! I'm going to try the closure, see what it looks like.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I ran out of time to make this look nice. Maybe when someone re-writes the Azure provisioner they can fix this...

@jwhitlock jwhitlock merged commit fa5d88d into taskcluster:main Dec 16, 2021
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.

3 participants