feat: add instrumentation to address error rates#600
Conversation
🦋 Changeset detectedLatest commit: 709a4d6 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #600 +/- ##
=======================================
Coverage 99.86% 99.86%
=======================================
Files 7 7
Lines 718 736 +18
=======================================
+ Hits 717 735 +18
Misses 1 1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
zimeg
left a comment
zimeg
left a comment
There was a problem hiding this comment.
🏛️ A few comments follow for the amazing reviewers!
| this.axios.defaults.headers.common["User-Agent"] = | ||
| `${packageJson.name.replace("/", ":")}/${packageJson.version} ` + | ||
| `axios/${this.axios.VERSION} ` + | ||
| `node/${process.version.replace("v", "")} ` + | ||
| `${os.platform()}/${os.release()}`; |
There was a problem hiding this comment.
🪬 thought: We should prefer the @slack/webhook package if support is added for Workflow Builder but for now we match the format:
There was a problem hiding this comment.
Good call 💯 recently we had to update the user agent from the web-api so that it is compliant with latin1, but I think this implementation does not need this patch since we aren't including the process.title
There was a problem hiding this comment.
@WilliamBergamin For this implementation let's keep node fixed instead of the process title but I do think we should mirror these changes upstream to @slack/webhook in future improvements 🎁
| it("adds metadata to webapi with package name and version", async () => { | ||
| const stub = sinon.stub(); | ||
| const original = Object.getOwnPropertyDescriptor( | ||
| webapi, | ||
| "addAppMetadata", | ||
| ); | ||
| Object.defineProperty(webapi, "addAppMetadata", { | ||
| value: stub, | ||
| configurable: true, | ||
| }); | ||
| try { | ||
| mocks.core.getInput.withArgs("method").returns("chat.postMessage"); | ||
| mocks.core.getInput.withArgs("token").returns("xoxb-example"); | ||
| new Config(mocks.core); | ||
| assert.ok(stub.calledOnce); | ||
| const { name, version } = stub.firstCall.args[0]; | ||
| assert.equal(name, "@slack/slack-github-action"); | ||
| assert.ok(version); | ||
| } finally { | ||
| Object.defineProperty(webapi, "addAppMetadata", original); | ||
| } | ||
| }); |
There was a problem hiding this comment.
🧪 note: We don't capture actual requests in tests at the moment so this is our current approach to checking expected behavior.
🔭 thought: Recent node improvements might allow us to improve how we test overall:
A little-known fact is that there is a builtin way to mock
fetch.
| @@ -1,5 +1,5 @@ | |||
| { | |||
| "name": "slack-github-action", | |||
| "name": "@slack/slack-github-action", | |||
There was a problem hiding this comment.
📣 note: We don't publish this package to npm but I still want to callout this change to match adjacent node packages:
@slack/bolt: https://github.com/slackapi/bolt-js/blob/81b2b3ed92e357571d8b845ef89901288cb5b9c2/package.json#L2-L2@slack/web-api: https://github.com/slackapi/node-slack-sdk/blob/4af912df43532331d17bddfd4f9c47157e5160ea/packages/web-api/package.json#L2@slack/webhook: https://github.com/slackapi/node-slack-sdk/blob/4af912df43532331d17bddfd4f9c47157e5160ea/packages/webhook/package.json#L2
There was a problem hiding this comment.
Good call 💯 praise 🙏
| @@ -1,5 +1,5 @@ | |||
| { | |||
| "name": "slack-github-action", | |||
| "name": "@slack/slack-github-action", | |||
There was a problem hiding this comment.
Good call 💯 praise 🙏
| this.axios.defaults.headers.common["User-Agent"] = | ||
| `${packageJson.name.replace("/", ":")}/${packageJson.version} ` + | ||
| `axios/${this.axios.VERSION} ` + | ||
| `node/${process.version.replace("v", "")} ` + | ||
| `${os.platform()}/${os.release()}`; |
There was a problem hiding this comment.
Good call 💯 recently we had to update the user agent from the web-api so that it is compliant with latin1, but I think this implementation does not need this patch since we aren't including the process.title
|
@WilliamBergamin Thank you for taking the time to review this PR 🙏 ✨ I'll merge this alongside recent package updates in #604 for upcoming release! 🚀 |
2 participants
Summary
This PR adds instrumentation to address error rates with a
User-Agentheader added to requests 🎹Preview
With additional logging in development to reveal we find these headers for different techniques:
API Method
Webhook
CLI
🎺 Instrumentation is left to adjacent implementations for installations and particular command usage.
Requirements