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

tag sort dedup #147

Merged
merged 7 commits into from
Jul 18, 2023
Merged

tag sort dedup #147

merged 7 commits into from
Jul 18, 2023

Conversation

extemporalgenome
Copy link
Contributor

@extemporalgenome extemporalgenome commented Jul 15, 2023

This is meant to be read commit-by-commit.

  • tag.go: conventional test naming.
  • add golang.org/x/exp/slices dependency
  • tag.go: use slices package Sort functionality
  • SortTags deduplicates, uses slices.SortStable

@extemporalgenome extemporalgenome force-pushed the tag-sort-dedup branch 7 times, most recently from 86a89ed to bdb182c Compare July 16, 2023 16:16
It looks like functions had been renamed in some files but not others,
breaking tests for darwin
(though it's unclear why darwin tests are valuable for this package).
Add golang.org/x/exp/slices dependency.

This is allocation-free and has comparable speed to prior approach.
stdlib and slices package Sort functions
also use insertion sort for small inputs,
and as such a local insertion sort optimization
has not been needed since at least 1.18.
golang.org/x/exp/slices has comparable allocation-free sorting,
which we can use while reducing our code.

Neither Datadog nor Prometheus appear to retain or handle
multiple tags with the same name on the same metric,
so this package deduplicates at time of sort (latest tag wins).

If any metric systems we use do tolerate and provide useful behavior
when provided duplicate tag/label names, and the practice is justified,
that would be a reason to revert or adjust this deduplication behavior.
return out
}

// mergeTags returns the sorted, deduplicated-by-name union of t1 and t2.
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you explain in the comment what strategy we use to deduplicate (last write wins).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The latest commit has hopefully addressed this. Please re-review.

- name: Run Tests
run: go test -race -tags=${{ matrix.tags }} ./...
- name: Identify Go Version
run: go version
Copy link
Contributor

@kevinburkesegment kevinburkesegment Jul 17, 2023

Choose a reason for hiding this comment

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

I believe each step starts a new container per command so there's a fair amount of overhead. How about e.g.

name: Identify Host 
run: |
    uname -a
    go version

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For BuildKite that'd definitely be the case, but for Github Actions, it looks like jobs create their own container/VM/whatever, but steps are just separate OS processes in that same container.

As such, steps look like they're no more expensive than running separate commands in a single shell. By making them separate steps, they do create nice UI sections though, and the above info was useful for debugging the especially stale workflow config.


- uses: actions/checkout@v3

- name: golangci-lint
uses: golangci/golangci-lint-action@v3.1.0
with:
version: v1.45.2
version: v1.53
Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for updating this.

@extemporalgenome extemporalgenome merged commit 1997ae9 into master Jul 18, 2023
4 checks passed
@extemporalgenome extemporalgenome deleted the tag-sort-dedup branch July 18, 2023 18:11
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