Correctly handle status compression on the server side #105
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
tigrannajaryan
commented
Jul 15, 2022
Codecov Report
@@ Coverage Diff @@
## main #105 +/- ##
==========================================
+ Coverage 74.95% 75.13% +0.18%
==========================================
Files 22 22
Lines 1633 1629 -4
==========================================
Hits 1224 1224
+ Misses 307 304 -3
+ Partials 102 101 -1
Continue to review full report at Codecov.
|
This was referenced Jul 15, 2022
Closed
|
This needs to be rebased after #110 is merged and then it can be reviewed. |
tigrannajaryan
force-pushed
the
feature/tigran/use-capabilities
branch
2 times, most recently
from
e1dbe01 to
a74a86d
Compare
tigrannajaryan
commented
Jul 22, 2022
| msg.Capabilities |= protobufs.AgentCapabilities_AcceptsPackages | ||
| msg.Capabilities |= protobufs.AgentCapabilities_ReportsPackageStatuses | ||
| } | ||
| msg.Capabilities = c.Capabilities |
There was a problem hiding this comment.
This ensures that the agent correctly reports the capabilities. I can extract this fix into a separate PR, but probably not worth it since it only became visible in this PR.
We check the omitted field and the corresponding capability bit to understand if the compression was used. Related to spec issue open-telemetry/opamp-spec#109 The issue was discarded in favour of the implementation that does not change the spec, but instead uses already existing information.
tigrannajaryan
force-pushed
the
feature/tigran/use-capabilities
branch
from
a74a86d to
0c16eea
Compare
|
@andykellr this is now ready for review. |
andykellr
approved these changes
Jul 28, 2022
PeterF778
reviewed
Aug 8, 2022
| // True if the status was not fully reported. | ||
| statusIsCompressed := effectiveConfigOmitted || packageStatusesOmitted || remoteConfigStatusOmitted || healthOmitted | ||
|
|
||
| if statusIsCompressed && lostPreviousUpdate { |
There was a problem hiding this comment.
No, "AND" is correct here. We only ned to ask for the full state if both of these conditions are true:
- The full state was not reported in the message we have just received. If this is not true, it means we have already just received the full state, there is no need to ask for it again.
- We detected that we lost a previous update. If this is not true, it means we already have the correct state on our end, there is no need to ask the client to send it again.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
We check the omitted field and the corresponding capability bit to understand
if the compression was used.
Related to spec issue open-telemetry/opamp-spec#109
The issue was discarded in favour of the implementation that does not
change the spec, but instead uses already existing information.