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.
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
Simplify SendPacket API #1703
Simplify SendPacket API #1703
Changes from 3 commits
05e1d86
99db505
a146b33
3d4a0eb
3cbbef6
847fd98
3527aed
7a94343
8386d5d
302d32a
8dc0c4a
60c740b
c6d9861
36742b0
e8b8a33
c348aa4
c30f011
079c3a1
90e5e08
827529a
e48d77e
acaf109
9a7737e
6ad76d1
2faf51e
17882e6
d9eb17f
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are we losing this telemetry info now for destination port and channel?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes. cc: @colin-axner
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it is okay, so long as there is telemetry within core IBC showing where packets are being sent from/to, then the telemetry could filter on port ID to get the same info. This might break external UX though, so might make more sense to do in a followup
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think telemetry is used on external UX? Also I just think it makes sense to keep it clean here, rather than grabbing channel just so that we can emit the same telemetry.
Afaiu, breaking telemetry does not count as an api breaking or state machine breaking change. So i don't want to be so restrictive and conservative in this case.
I'm in favor of just removing it. We can add it in the changelog so that people who need this information are informed of it beforehand
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changing any of the labels will break the telemetry counter. Thus any IBC apps reporting statistics via Prometheus would likely break. I think it makes sense to check with external apps to ensure we don't make a disruptive change out of the blue
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could remove this telemetry entirely and replace it with one in core IBC that emits per packet sent, rather than duplicating for every application
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.