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

Single module #269

Closed
wants to merge 11 commits into from
Closed

Single module #269

wants to merge 11 commits into from

Conversation

jsirianni
Copy link
Member

Description of Changes

Converted Stanza to a single module. Managing multiple modules complicates the release process. Now that Stanza's core has been implemented into Open Telemetry, we can safely manage Stanza as a single module.

This meant the existing versioning system would need to be replaced. I opted for variable injection at build time by leveraging GIT_TAG and GIT_COMMIT environment variables.

Makefile will set GIT_COMMIT.

➜  stanza git:(single-module) ✗ make build-darwin-amd64
➜  stanza git:(single-module) ✗ artifacts/stanza_darwin_amd64 version

git-commit: 7b83971df04c378599515fef09ef895bbdf5170d

If GIT_TAG is set, it will take precedence over GIT_COMMIT

➜  stanza git:(single-module) ✗ export GIT_TAG=v1.2
➜  stanza git:(single-module) ✗ make build-darwin-amd64
➜  stanza git:(single-module) ✗ artifacts/stanza_darwin_amd64 version

v1.2

TODO: Determine the new Stanza version and update the changelog. I believe this is technically not a breaking change, individual packages can still be imported and used by external applications.

Please check that the PR fulfills these requirements

  • Tests for the changes have been added (for bug fixes / features)
  • Docs have been added / updated (for bug fixes / features)
  • Add a changelog entry (for non-trivial bug fixes / features)
  • CI passes

…at build time. Instead, inject a tag or commit during build time. GitTag will be used if set, falling back on GitCommit
@jsirianni jsirianni requested a review from djaglowski April 5, 2021 22:48
@djaglowski
Copy link
Member

Log Files Logs / Second CPU Avg (%) CPU Avg Δ (%) Memory Avg (MB) Memory Avg Δ (MB)
1 1000 1.8276136 -0.17246532 124.01738 -0.8621979
1 5000 6.1380744 +0.06893587 133.4798 +3.4924622
1 10000 11.448505 -1.2244825 140.25809 +1.1104584
1 50000 56.656136 -1.4330254 169.29984 -3.5517273
1 100000 117.06489 +0.9588547 222.11018 -12.551453
10 100 2.2586455 -0.29324913 127.75808 -0.8351364
10 500 6.724317 -0.63792753 131.06169 -1.202591
10 1000 12.914254 -1.0172434 140.24232 +0.5964508
10 5000 59.501625 -3.0691185 175.76993 -5.742874
10 10000 130.39795 +2.4485092 221.52829 -8.49852

@djaglowski
Copy link
Member

This meant the existing versioning system would need to be replaced. I opted for variable injection at build time by leveraging GIT_TAG and GIT_COMMIT environment variables.

Can you explain this further? Certainly we would need to increment the version of stanza, but I'm not clear on why the whole versioning system needs to be reworked. Are you sure?

@jsirianni
Copy link
Member Author

This meant the existing versioning system would need to be replaced. I opted for variable injection at build time by leveraging GIT_TAG and GIT_COMMIT environment variables.

Can you explain this further? Certainly we would need to increment the version of stanza, but I'm not clear on why the whole versioning system needs to be reworked. Are you sure?

We would get the stanza module version from cmd/stanza/go.mod

github.com/observiq/stanza v0.13.18

We used a package to read the module version when calling stanza version. Now, if you add stanza to go.mod, it will get removed during the build.

➜  stanza git:(single-module) ✗ make build-darwin-amd64
(cd ./cmd/stanza && \
		CGO_ENABLED=0 \
		go build \
		-ldflags "-X github.com/observiq/stanza/version.GitTag= -X github.com/observiq/stanza/version.GitCommit=573b7aad2b8db684657bc8afecd5d68b199dd1e7" \
		-o ../../artifacts/stanza_darwin_amd64  .)
go: github.com/observiq/[email protected]: missing go.sum entry; to add it:
	go mod download github.com/observiq/stanza
make[1]: *** [build] Error 1
make: *** [build-darwin-amd64] Error 2
➜  stanza git:(single-module) ✗ go mod download github.com/observiq/stanza
go mod download: skipping argument github.com/observiq/stanza that resolves to the main module

We could set the version in the version package, instead of relying on the module package (and avoid the build time variable injection). I don't have a strong opinion on either option.

@djaglowski
Copy link
Member

Thanks for explaining. I see what you're saying.

I think we should look for a way to use v0.0.0 format, since that's very standard in Go. Certainly the module needs to follow this format, so it would make sense that the agent itself should have the same version.

Ideally, the release process we have (creating a github release) would set the version based on the git tag (v0.0.0). This should be doable in the CI workflow, and would mean that it's the only place where the version is actually set. Builds created locally would not be versioned, or could be versioned as you have now.

Thoughts on this approach?

@jsirianni
Copy link
Member Author

Thanks for explaining. I see what you're saying.

I think we should look for a way to use v0.0.0 format, since that's very standard in Go. Certainly the module needs to follow this format, so it would make sense that the agent itself should have the same version.

Ideally, the release process we have (creating a github release) would set the version based on the git tag (v0.0.0). This should be doable in the CI workflow, and would mean that it's the only place where the version is actually set. Builds created locally would not be versioned, or could be versioned as you have now.

Thoughts on this approach?

I believe this is exactly what I am doing. When running in CircleCI, we will use the git tag. So the same release process exists:

  1. Create draft release
  2. Let Circle build the release and tag with v0.0.0
  3. Publish the release
  4. stanza version will return the git tag v0.0.0.

I added GIT_COMMIT versioning for local builds only. If you run make build without GIT_TAG set in your environment, The Makefile will inject GIT_COMMIT for the version. This could easily be removed.

Copy link
Member

@djaglowski djaglowski left a comment

Choose a reason for hiding this comment

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

Got it. Sorry I missed that.

LGTM (assuming unit tests are fixed)

@djaglowski
Copy link
Member

Log Files Logs / Second CPU Avg (%) CPU Avg Δ (%) Memory Avg (MB) Memory Avg Δ (MB)
1 1000 1.7069365 -0.15515399 124.46942 +0.8659744
1 5000 5.827823 -0.20676613 131.42726 -1.0610199
1 10000 12.051929 -0.25874424 139.21431 -2.6317291
1 50000 56.78919 -2.8683777 167.88632 -5.6649933
1 100000 114.430305 -5.450737 233.05092 +10.48317
10 100 2.4310277 -0.06904268 127.57907 -0.5389252
10 500 6.8278246 -0.103385925 133.19894 -2.7606506
10 1000 13.638185 -0.08621311 142.23276 -1.4251099
10 5000 61.4509 -2.2573318 180.94249 -4.422943
10 10000 126.95308 +1.4360504 225.83122 -1.624054

@djaglowski
Copy link
Member

Log Files Logs / Second CPU Avg (%) CPU Avg Δ (%) Memory Avg (MB) Memory Avg Δ (MB)
1 1000 1.6896697 -0.17242074 124.23033 +0.62688446
1 5000 6.1207957 +0.086206436 131.1448 -1.3434753
1 10000 12.000399 -0.31027508 140.09322 -1.7528229
1 50000 56.77861 -2.8789558 173.07893 -0.4723816
1 100000 113.352295 -6.5287476 232.20918 +9.641434
10 100 2.3104699 -0.18960047 126.171875 -1.9461212
10 500 6.620827 -0.31038332 132.94316 -3.0164337
10 1000 13.914119 +0.18972111 140.86989 -2.7879791
10 5000 62.86525 -0.84298325 178.40045 -6.964981
10 10000 116.73685 -8.780182 225.06236 -2.3929138

@djaglowski
Copy link
Member

Log Files Logs / Second CPU Avg (%) CPU Avg Δ (%) Memory Avg (MB) Memory Avg Δ (MB)
1 1000 1.8965884 +0.034497976 122.75014 -0.85330963
1 5000 6.3278046 +0.29321527 133.86058 +1.3722992
1 10000 11.793384 -0.5172901 138.63632 -3.2097168
1 50000 60.349167 +0.6916008 179.97064 +6.419327
1 100000 116.244965 -3.636078 229.71794 +7.1501923
10 100 2.4483266 -0.051743746 129.37634 +1.2583466
10 500 6.9485426 +0.017332077 133.91959 -2.0400085
10 1000 13.36273 -0.36166763 142.43158 -1.2262878
10 5000 61.55129 -2.1569443 177.14574 -8.219696
10 10000 124.44632 -1.0707092 218.36827 -9.087006

@djaglowski
Copy link
Member

Log Files Logs / Second CPU Avg (%) CPU Avg Δ (%) Memory Avg (MB) Memory Avg Δ (MB)
1 1000 1.6207153 -0.24137521 122.538795 -1.0646515
1 5000 5.9140663 -0.120522976 132.533 +0.04472351
1 10000 12.086654 -0.22402 138.67888 -3.16716
1 50000 59.140755 -0.5168114 172.23357 -1.317749
1 100000 113.81779 -6.0632553 223.20784 +0.64009094
10 100 2.3621655 -0.13790488 128.66797 +0.54997253
10 500 6.844973 -0.08623743 133.67754 -2.2820587
10 1000 13.621112 -0.10328579 143.91205 +0.2541809
10 5000 61.259983 -2.4482498 178.17606 -7.189377
10 10000 122.849724 -2.667305 222.74852 -4.7067566

@jsirianni
Copy link
Member Author

Closing for now. I am not sure what is causing the Linux failures. I will circle back in the future.

@jsirianni jsirianni closed this Apr 13, 2021
@jsirianni jsirianni deleted the single-module branch May 18, 2021 21:20
@jsirianni jsirianni mentioned this pull request May 18, 2021
4 tasks
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.

2 participants