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

fix: crrect initialization of a few slices #12674

Merged
merged 2 commits into from
Apr 18, 2024

Conversation

seiyab
Copy link
Contributor

@seiyab seiyab commented Apr 18, 2024

What this PR does / why we need it:
I found suspicious slice initialization so fixed them.
These slices are initialized by make([]type, size). But their contents are filled by slice = append(slice, newElem). It should result leading zero values.
Say,

xs := make([]int, 3)
xs = append(xs, 1)
xs = append(xs, 2)
xs = append(xs, 3)

will result xs = []int{0, 0, 0, 1, 2, 3}. Probably it's unintentional and means

xs := make([]int, 0, 3)
xs = append(xs, 1)
xs = append(xs, 2)
xs = append(xs, 3)

Which issue(s) this PR fixes:
N/A. I consider it's a trivial fix.

Special notes for your reviewer:
Feel free to reject this PR.I'm sorry but I'm not familiar with Loki. I found it just scanning Loki with my experimental static checker.
I've read codes around these lines and callers of them. I suppose that they have confusion between length and capacity but I don't understand what they do in context of entire application.
Anyway, I suppose leading zero values look unintentional and tests pass.

Checklist

  • Reviewed the CONTRIBUTING.md guide (required)
  • Documentation added
  • Tests updated
    • Need you help if new tests are required
  • Title matches the required conventional commits format, see here
  • Changes that require user attention or interaction to upgrade are documented in docs/sources/setup/upgrade/_index.md
    • Need your help. I'm not sure. I guess it's not user-facing because tests passes without any change. My hypothesis is that zero values are dropped implicitly by callers.
  • For Helm chart changes bump the Helm chart version in production/helm/loki/Chart.yaml and update production/helm/loki/CHANGELOG.md and production/helm/loki/README.md. Example PR
  • If the change is deprecating or removing a configuration option, update the deprecated-config.yaml and deleted-config.yaml files respectively in the tools/deprecated-config-checker directory. Example PR

@seiyab seiyab requested a review from a team as a code owner April 18, 2024 14:29
@CLAassistant
Copy link

CLAassistant commented Apr 18, 2024

CLA assistant check
All committers have signed the CLA.

@@ -269,7 +269,7 @@ func (q *IngesterQuerier) TailersCount(ctx context.Context) ([]uint32, error) {
return nil, err
}

counts := make([]uint32, len(responses))
Copy link
Contributor

Choose a reason for hiding this comment

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

this seems like a good change, AFAICT atm we could be allocating # of ingesters * 2 uint32's since we could have a response from every ingester

@@ -305,7 +305,7 @@ func (c *IndexReaderWriter) chunksToSeries(ctx context.Context, in []logproto.Ch
return nil, err
}

results := make([]labels.Labels, len(chunksBySeries)) // Flatten out the per-job results.
results := make([]labels.Labels, 0, len(chunksBySeries)) // Flatten out the per-job results.
Copy link
Contributor

Choose a reason for hiding this comment

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

this also seems safe, but actually I think it should be len(perJobResults) should it not? @bboreham I'm not sure from your original PR (here) if you intended to create the slice of chunkBySeries length rather than perJobResults length

Copy link
Contributor Author

@seiyab seiyab Apr 18, 2024

Choose a reason for hiding this comment

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

No, len(chunksBySeries) looks correct. I also feel len(perJobResults) is correct at first sight.
Actually, argument of append is innerSlice..., not innerSlice. And sum of their length is probably len(chunkBySeries) (groups := make([]chunkGroup, 0, len(chunksBySeries)/c.chunkBatchSize+1).

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah you're right, I misread the append line 👍 good find

@cstyan cstyan merged commit 0eba448 into grafana:main Apr 18, 2024
14 checks passed
@seiyab
Copy link
Contributor Author

seiyab commented Apr 18, 2024

Thank you for kindful & speedy review!

@seiyab seiyab deleted the correct-slice-initialization branch April 18, 2024 22:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants