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

Proper enrichFromCache panic fix and go-whisper upgrade #486

Merged
merged 8 commits into from Aug 11, 2022

Conversation

deniszh
Copy link
Member

@deniszh deniszh commented Aug 11, 2022

  • proper fix for enrichFromCache panic. Extending array with 0, tried NaN but it's immediately failing during marshalling, obviously.
  • upgrading go-whisper to include buffer merge fix.

@deniszh
Copy link
Member Author

deniszh commented Aug 11, 2022

Not sure why linter doing this... I see 0 issues running it locally:

± golangci-lint run --verbose --enable gofmt,goimports,gocritic --disable errcheck ./...
INFO [config_reader] Config search paths: [./  /]
INFO [lintersdb] Active 11 linters: [deadcode gocritic gofmt goimports gosimple govet ineffassign staticcheck typecheck unused varcheck]
INFO [loader] Go packages loading at mode 575 (compiled_files|deps|files|imports|exports_file|name|types_sizes) took 658.273375ms
INFO [runner/filename_unadjuster] Pre-built 0 adjustments in 9.986167ms
INFO [linters context/goanalysis] analyzers took 0s with no stages
INFO [runner] Issues before processing: 44, after processing: 0
INFO [runner] Processors filtering stat (out/in): autogenerated_exclude: 23/44, cgo: 44/44, filename_unadjuster: 44/44, path_prettifier: 44/44, skip_files: 44/44, identifier_marker: 23/23, exclude: 23/23, nolint: 0/23, skip_dirs: 44/44, exclude-rules: 23/23
INFO [runner] processing took 11.02004ms with stages: nolint: 9.110417ms, autogenerated_exclude: 701.25µs, exclude-rules: 698.958µs, identifier_marker: 265.957µs, path_prettifier: 188.416µs, skip_dirs: 45.875µs, cgo: 4.542µs, filename_unadjuster: 1.999µs, max_same_issues: 749ns, exclude: 292ns, severity-rules: 251ns, max_per_file_from_linter: 251ns, max_from_linter: 250ns, uniq_by_line: 208ns, diff: 166ns, path_shortener: 125ns, source_code: 125ns, skip_files: 84ns, sort_results: 84ns, path_prefixer: 41ns
INFO [runner] linters took 184.110125ms with stages: goanalysis_metalinter: 173.043333ms
INFO File cache stats: 0 entries of total size 0B
INFO Memory: 10 samples, avg is 52.8MB, max is 52.8MB
INFO Execution took 861.176084ms

@deniszh deniszh requested a review from emadolsky August 11, 2022 12:16
@bom-d-van
Copy link
Member

hi, @deniszh in case the comment didn't reach you, the changes in go-whisper might have failed some of the unit test cases. more here: go-graphite/go-whisper#35 (comment)

for _, item := range cacheData {
ts := int64(item.Timestamp) - int64(item.Timestamp)%r.StepTime
if ts < r.StartTime || ts >= r.StopTime {
continue
}
pointsFetchedFromCache++
index := (ts - r.StartTime) / r.StepTime
// TODO: log.debug such cases
if index >= 0 && index < int64(len(r.Values)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

How about keeping the defensive code just in case?
This way, a bug or invalid data in the cache (like ts older than r.StartTime) will not cause a panic.
But I'm not well aware of the code as a whole. So your call.

Copy link
Member Author

Choose a reason for hiding this comment

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

Well, it will not hurt, so let's keep it.

@deniszh
Copy link
Member Author

deniszh commented Aug 11, 2022

@bom-d-van : Thanks, it's fixed now!

@deniszh deniszh merged commit b2b610e into go-graphite:master Aug 11, 2022
@deniszh deniszh deleted the dzhdanov/enrichFromCache-inv branch August 11, 2022 13:36
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.

None yet

3 participants