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

Simplify status compression #93

Merged
merged 1 commit into from
Jun 29, 2022
Merged

Simplify status compression #93

merged 1 commit into from
Jun 29, 2022

Conversation

tigrannajaryan
Copy link
Member

@tigrannajaryan tigrannajaryan commented Jun 29, 2022 •
edited
Loading

  • Eliminate all "hash" fields.
  • Allow omitting the sub-messages of AgentToServer message. When omitted it is implied that previously reported value of the sub-message is current (unchanged).
  • To detect lost messages have one auto-incremented sequence_num field AgentToServer message. Server can easily detect losses by just keeping the last sequence_num (as opposed to keeping 4 different hashes).

Implements spec change open-telemetry/opamp-spec#101

@tigrannajaryan tigrannajaryan mentioned this pull request Jun 29, 2022
Closed
@codecov
Copy link

codecov bot commented Jun 29, 2022 •
edited
Loading

Codecov Report

Merging #93 (1f70451) into main (301ef45) will decrease coverage by 1.63%.
The diff coverage is 90.90%.

❗ Current head 1f70451 differs from pull request most recent head 471ea2d. Consider uploading reports for the commit 471ea2d to get more accurate results

@@            Coverage Diff             @@
##             main      #93      +/-   ##
==========================================
- Coverage   76.15%   74.51%   -1.64%     
==========================================
  Files          22       22              
  Lines        1661     1562      -99     
==========================================
- Hits         1265     1164     -101     
- Misses        297      304       +7     
+ Partials       99       94       -5
Impacted Files Coverage Δ
client/internal/clientstate.go 65.62% <ø> (-15.96%) ⬇️
client/internal/receivedprocessor.go 79.24% <85.71%> (-2.01%) ⬇️
client/internal/clientcommon.go 79.65% <100.00%> (-2.94%) ⬇️
client/internal/nextmessage.go 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 301ef45...471ea2d. Read the comment docs.

@tigrannajaryan tigrannajaryan force-pushed the feature/tigran/new-compr branch from 1f70451 to 9b529f7 Compare June 29, 2022 16:27
@tigrannajaryan tigrannajaryan marked this pull request as ready for review June 29, 2022 16:28
@tigrannajaryan tigrannajaryan requested review from a team and andykellr June 29, 2022 16:28
tigrannajaryan added a commit to tigrannajaryan/opamp-spec that referenced this pull request Jun 29, 2022
@tigrannajaryan
Simplify status compression
def752c 
Resolves open-telemetry#101

Status compression was quite complicated and confusing. It used 4 different
hashes and lots of code to enable compression and detection of lost messages
(during server restarts).

We simplified this significantly by doing this instead:
- Eliminate all "hash" fields.
- Allow omitting the sub-messages of AgentToServer message. When omitted it is implied that previously reported value of the sub-message is current (unchanged).
- To detect lost messages have one auto-incremented sequence_num field AgentToServer message. Server can easily detect losses by just keeping the last sequence_num (as opposed to keeping 4 different hashes).

Here is a PR that demonstrates how it works:
open-telemetry/opamp-go#93

It is more than 100 lines of code removed.
tigrannajaryan added a commit to tigrannajaryan/opamp-spec that referenced this pull request Jun 29, 2022
@tigrannajaryan
Simplify status compression
b737e6c 
Resolves open-telemetry#101

Status compression was quite complicated and confusing. It used 4 different
hashes and lots of code to enable compression and detection of lost messages
(during server restarts).

We simplified this significantly by doing this instead:
- Eliminate all "hash" fields.
- Allow omitting the sub-messages of AgentToServer message. When omitted it is implied that previously reported value of the sub-message is current (unchanged).
- To detect lost messages have one auto-incremented sequence_num field AgentToServer message. Server can easily detect losses by just keeping the last sequence_num (as opposed to keeping 4 different hashes).

Here is a PR that demonstrates how it works:
open-telemetry/opamp-go#93

It is more than 100 lines of code removed.
tigrannajaryan added a commit to tigrannajaryan/opamp-spec that referenced this pull request Jun 29, 2022
@tigrannajaryan
Simplify status compression
3b40727 
Resolves open-telemetry#101

Status compression was quite complicated and confusing. It used 4 different
hashes and lots of code to enable compression and detection of lost messages
(during server restarts).

We simplified this significantly by doing this instead:
- Eliminate all "hash" fields.
- Allow omitting the sub-messages of AgentToServer message. When omitted it is implied that previously reported value of the sub-message is current (unchanged).
- To detect lost messages have one auto-incremented sequence_num field AgentToServer message. Server can easily detect losses by just keeping the last sequence_num (as opposed to keeping 4 different hashes).

Here is a PR that demonstrates how it works:
open-telemetry/opamp-go#93

It is more than 100 lines of code removed.
@tigrannajaryan tigrannajaryan mentioned this pull request Jun 29, 2022
Merged
tigrannajaryan added a commit to tigrannajaryan/opamp-spec that referenced this pull request Jun 29, 2022
@tigrannajaryan
Simplify status compression
ae37f74 
Resolves open-telemetry#101

Status compression was quite complicated and confusing. It used 4 different
hashes and lots of code to enable compression and detection of lost messages
(during server restarts).

We simplified this significantly by doing this instead:
- Eliminate all "hash" fields.
- Allow omitting the sub-messages of AgentToServer message. When omitted it is implied that previously reported value of the sub-message is current (unchanged).
- To detect lost messages have one auto-incremented sequence_num field AgentToServer message. Server can easily detect losses by just keeping the last sequence_num (as opposed to keeping 4 different hashes).

Here is a PR that demonstrates how it works:
open-telemetry/opamp-go#93

It is more than 100 lines of code removed.
tigrannajaryan added a commit to open-telemetry/opamp-spec that referenced this pull request Jun 29, 2022
@tigrannajaryan
Simplify status compression (#102)
1f56f86 
Resolves #101

Status compression was quite complicated and confusing. It used 4 different
hashes and lots of code to enable compression and detection of lost messages
(during server restarts).

We simplified this significantly by doing this instead:
- Eliminate all "hash" fields.
- Allow omitting the sub-messages of AgentToServer message. When omitted it is implied that previously reported value of the sub-message is current (unchanged).
- To detect lost messages have one auto-incremented sequence_num field AgentToServer message. Server can easily detect losses by just keeping the last sequence_num (as opposed to keeping 4 different hashes).

Here is a PR that demonstrates how it works:
open-telemetry/opamp-go#93

It is more than 100 lines of code removed.
andykellr
andykellr requested changes Jun 29, 2022
View reviewed changes
Copy link
Contributor

@andykellr andykellr left a comment

Choose a reason for hiding this comment

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

One comment needs to be adjusted. Otherwise I think this looks great.

@andykellr
Copy link
Contributor

Actually, doing a search for Hash I found a couple more places that should be updated. I'll add suggestions.

@tigrannajaryan tigrannajaryan force-pushed the feature/tigran/new-compr branch from 9b529f7 to ad590db Compare June 29, 2022 18:12
@tigrannajaryan
Simplify status compression
471ea2d 
- Eliminate all "hash" fields.
- Allow omitting the sub-messages of AgentToServer message. When omitted it is implied that previously reported value of the sub-message is current (unchanged).
- To detect lost messages have one auto-incremented sequence_num field AgentToServer message. Server can easily detect losses by just keeping the last sequence_num (as opposed to keeping 4 different hashes).
@andykellr
Copy link
Contributor

The following have comments that refer to the Hash field:

  • client/types/startsettings.go
  • client/types/callbacks.go
  • client/client.go

@tigrannajaryan tigrannajaryan force-pushed the feature/tigran/new-compr branch from ad590db to 471ea2d Compare June 29, 2022 18:18
@tigrannajaryan
Copy link
Member Author

The following have comments that refer to the Hash field:

  • client/types/startsettings.go
  • client/types/callbacks.go
  • client/client.go

Thanks for the catch. I did a pass over this files. Please check again.

@tigrannajaryan tigrannajaryan requested a review from andykellr June 29, 2022 18:20
andykellr
andykellr approved these changes Jun 29, 2022
View reviewed changes
Copy link
Contributor

@andykellr andykellr left a comment

Choose a reason for hiding this comment

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

Looks good, thanks.

@tigrannajaryan tigrannajaryan merged commit aad0403 into open-telemetry:main Jun 29, 2022
@tigrannajaryan tigrannajaryan deleted the feature/tigran/new-compr branch June 29, 2022 18:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@andykellr andykellr andykellr 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.
2 participants
@tigrannajaryan @andykellr