Skip to content

Conversation

@zimeg
Copy link
Member

@zimeg zimeg commented Jun 12, 2025

Summary

This PR adds the target architecture of compiled builds in events sent to logstash for errors following #101 #114 #124.

Reviewers

Confirm this build includes the target machine's architecture with the following command:

$ slack version --verbose
...
FlushToLogstash will POST https://dev.slackb.com/events/cli payload: [{"context":{"arch":"arm64","...":"..."}]

Requirements

@zimeg zimeg added this to the Next Release milestone Jun 12, 2025
@zimeg zimeg self-assigned this Jun 12, 2025
@zimeg zimeg requested a review from a team as a code owner June 12, 2025 22:00
@zimeg zimeg added enhancement M-T: A feature request for new functionality semver:patch Use on pull requests to describe the release version increment labels Jun 12, 2025
@codecov
Copy link

codecov bot commented Jun 12, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 63.49%. Comparing base (41cf0f5) to head (5113c2a).
Report is 1 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #126      +/-   ##
==========================================
+ Coverage   63.48%   63.49%   +0.01%     
==========================================
  Files         212      212              
  Lines       22344    22345       +1     
==========================================
+ Hits        14184    14188       +4     
+ Misses       7078     7077       -1     
+ Partials     1082     1080       -2     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Copy link
Member Author

@zimeg zimeg left a comment

Choose a reason for hiding this comment

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

🪵 Words of findings and thought in these changes for the kind reviewers!

CLIVersion: versionString,
Host: ioutils.GetHostname(),
OS: runtime.GOOS,
Arch: runtime.GOARCH,
Copy link
Member Author

Choose a reason for hiding this comment

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

📣 TIL that GOARCH is set at compile time and not runtime, so this matches the installed build:

🔗 https://pkg.go.dev/runtime

GOARCH, GOOS, and GOROOT are recorded at compile time and made available by constants or functions in this package, but they do not influence the execution of the run-time system.

Copy link
Member

Choose a reason for hiding this comment

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

Wow! I was about to point that this is may report the user's system architecture instead of the binary's architecture. Thanks for digging into it and finding out that it uses the binary's arch!

Copy link
Member

@mwbrooks mwbrooks left a comment

Choose a reason for hiding this comment

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

✅ Great work @zimeg! This will definitely help us understand what architecture folks are running.

🧪 I've manually tested by downloading the Universal and Intel binaries and confirming that the logs are flusehd with amd64 arch:

# Remove the unsigned warning
$ xattr -dr com.apple.quarantine ./bin/slack

# Test
./bin/slack version --verbose

# [2025-06-12 15:34:45] FlushToLogstash will POST https://dev.slackb.com/events/cli payload: [{"context":{"arch":"amd64","bin":"./bin/slack","cli_version":"3.3.0-15-g5113c2a","command":"version","command_canonical":"version","flags":["verbose"],"host":"...","os":"darwin","session_id":"...","system_id":"..."},"data":{"app":{},"auth":{}},"event":"success","timestamp":...}]

CLIVersion: versionString,
Host: ioutils.GetHostname(),
OS: runtime.GOOS,
Arch: runtime.GOARCH,
Copy link
Member

Choose a reason for hiding this comment

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

Wow! I was about to point that this is may report the user's system architecture instead of the binary's architecture. Thanks for digging into it and finding out that it uses the binary's arch!

@zimeg
Copy link
Member Author

zimeg commented Jun 12, 2025

@mwbrooks Thanks for checking the releases! I didn't think to also test with the universal build but found arm64 on my machine! 📠

The amd64 build however outputs what we're both expecting so I will merge this.

But beforehand, a thankful TIL about this most helpful command - I have been using System Preferences:

$ xattr -dr com.apple.quarantine ./bin/slack

@zimeg zimeg merged commit 1f2d121 into main Jun 12, 2025
6 checks passed
@zimeg zimeg deleted the zimeg-feat-logstash-target-arch branch June 12, 2025 22:46
@mwbrooks
Copy link
Member

@zimeg Yea, it's unfortunate that the Universal binary is not reporting both architectures or something unique, but we have to work with what we have!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement M-T: A feature request for new functionality semver:patch Use on pull requests to describe the release version increment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants