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.

Sign up for GitHub

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

Jump to bottom

Reformat offsets_result.json for brevity and better human visibility #45

Merged
merged 7 commits into from
Apr 28, 2023
Merged

Reformat offsets_result.json for brevity and better human visibility #45

merged 7 commits into from
Apr 28, 2023

Conversation

mariomac
Copy link
Contributor

This PR proposes a change in the format of the offsets_restuls.json files:

  • Does not store all the versions from all the libraries but only the versions that change a given offset through its predecessor.
  • It replaces some arrays by maps so you can lookup directly structs and fields without having to iterate.

The arguable advantages:

  • Shorter length of the file (from 6K lines to 182).
  • Resulting file is more "human-friendly". Being simpler to visually locate offsets and changes across versions.
  • Most times will work when there is a new version of a library and the offsets haven't been yet updated. Most of the times the offset will remain the same, and the current version of Go or a Library will match to the newest library in the file.

@mariomac mariomac requested a review from a team March 23, 2023 09:51
@mariomac mariomac changed the title Shorter offsets_result.json Reformat offsets_result.json for brevity and better human visibility Mar 23, 2023
dineshg13
dineshg13 reviewed Mar 31, 2023
View reviewed changes
offsets-tracker/go.mod Outdated Show resolved Hide resolved
@MrAlias
Copy link
Contributor

MrAlias commented Apr 18, 2023

@mariomac any chance you can sync with main, or would you want to recreate this?

@mariomac
Copy link
Contributor Author

@MrAlias done!

@damemi
Copy link
Contributor

damemi commented Apr 19, 2023

@mariomac offsets check failed, looks like it's partially due to #84 but also some go 1.20.2/1.20.3 diffs. Can you try running make docker-offsets?

@mariomac
Copy link
Contributor Author

@damemi Ooops... sorry I forgot it!

@MrAlias MrAlias merged commit d6ec5c3 into open-telemetry:main Apr 28, 2023
@mariomac mariomac deleted the shorten branch April 28, 2023 21:21
MrAlias referenced this pull request in MrAlias/opentelemetry-go-instrumentation Apr 28, 2023
@mariomac @damemi @MrAlias
Reformat offsets_result.json for brevity and better human visibility (#…
7420123 
…45)

* Shorter offsets_result.json

* updated offsets_resuts.json

* Add changelog comment

---------

Co-authored-by: Mike Dame <[email protected]>
Co-authored-by: Tyler Yahn <[email protected]>
@MrAlias MrAlias mentioned this pull request May 3, 2023
Merged
@mariomac mariomac mentioned this pull request May 4, 2023
Closed
@MrAlias MrAlias mentioned this pull request Oct 13, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dineshg13 dineshg13 dineshg13 left review comments

@MrAlias MrAlias MrAlias approved these changes

@MikeGoldsmith MikeGoldsmith MikeGoldsmith approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@mariomac @MrAlias @damemi @dineshg13 @MikeGoldsmith