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

[ResponseOps] URLs longer than 4K bytes are split in Slack and email messages #158262

Open
pmuellr opened this issue May 23, 2023 · 10 comments
Open
Labels
bug Fixes for quality problems that affect the customer experience Feature:Alerting Team:ResponseOps Label for the ResponseOps team (formerly the Cases and Alerting teams)

Comments

@pmuellr
Copy link
Member

pmuellr commented May 23, 2023

I did a test with the Slack connector, sending http://example.com/app/r?l=DISCOVER_APP_LOCATOR&v=8.7.1&lz=1234567890 with the 1..0 numbers repeated 400 times. Total of 4060 bytes. This is from a support case, for a use of {{context.link}} variable from an elasticsearch query rule in an action.

Slack split it into 3992 bytes in the first message, remainder in the second. We've seen that Slack has a 40K message limit, but this is below that. It's possible the 40K limit is the total limit of the message, but that there's a ~4K limit on line lengths, or something. I didn't see any mention of possible line length limits.

This was a change from 8.6.2 to 8.7.1, for one user's rule we looked at, where the URL went from 254 bytes to 1072 bytes. The same customer hit the "split line" problem with a URL > 4000 bytes.

Seems like the change to the URL was made here: #146403

Generating links in the "saved objects" format will result in much smaller URLs, however that also means each of those queries will be persisted in an index, forever.

We'll need to figure out some story for these ...

@pmuellr pmuellr added bug Fixes for quality problems that affect the customer experience Feature:Alerting Team:ResponseOps Label for the ResponseOps team (formerly the Cases and Alerting teams) labels May 23, 2023
@elasticmachine
Copy link
Contributor

Pinging @elastic/response-ops (Team:ResponseOps)

@pmuellr
Copy link
Member Author

pmuellr commented May 25, 2023

We're now seeing some issues with emails as well. One email we saw was truncated at 2048 bytes. Not out of the question that broke some email processor's url-length-limit, or something, especially in an email.

@pmuellr pmuellr added the Team:DataDiscovery Discover, search (e.g. data plugin and KQL), data views, saved searches. For ES|QL, use Team:ES|QL. label May 25, 2023
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-data-discovery (Team:DataDiscovery)

@pmuellr
Copy link
Member Author

pmuellr commented May 25, 2023

Added the data discovery team, as we will eventually have to depend on them for a solution here.

We obviously know the lengths of these URLs when we create them, so it seems like we can do something at the alerting side - when we actually know a mustache variable is a URL - but generally only rule types know what their mustache variables are. So, if it's less than some limit (say 1K), continue writing out the zipped-uuencoded-JSON version as a query string version of the URL, and if it's not ... something else

We could try making these "short URLs" by using the existing SO's where short links get saved. But since these will just pile up over time, perhaps we could put a configurable TTL on them - two weeks default or so.

@pmuellr pmuellr moved this from Awaiting Triage to Todo in AppEx: ResponseOps - Execution & Connectors May 25, 2023
@pmuellr pmuellr changed the title [ResponseOps] URLs longer than 4K bytes are split in Slack messages [ResponseOps] URLs longer than 4K bytes are split in Slack and email messages May 25, 2023
@kertal
Copy link
Member

kertal commented May 30, 2023

We could try making these "short URLs" by using the existing SO's where short links get saved. But since these will just pile up over time, perhaps we could put a configurable TTL on them - two weeks default or so.

yes I think TTL for such URLs would be a nice feature FYI @elastic/appex-sharedux
@jughosta is working on a quick fix that will bring help to cases when the URL gets very long due to data views with lots of field attributes.
@pmuellr like you mention we could generate only generate short URLs when the length of a serialized encoded exceeds a certain limit, then the link would work in any case

@mikecote
Copy link
Contributor

@jughosta is working on a quick fix that will bring help to cases when the URL gets very long due to data views with lots of field attributes.

@kertal do you think the quick fix will be sufficient to close this GitHub issue afterwards? I can link the PR as resolving this issue or do you think there is leftover work that should be tracked with this issue?

jughosta added a commit that referenced this issue May 31, 2023
- Addresses #158262

## Summary

This PR makes alert links shorter by removing redundant props from the
encoded state. We should trim it down more in the future. Backporting a
small fix for now.

For testing:
Please follow instructions from this PR description
#146403
kibanamachine pushed a commit to kibanamachine/kibana that referenced this issue May 31, 2023
- Addresses elastic#158262

## Summary

This PR makes alert links shorter by removing redundant props from the
encoded state. We should trim it down more in the future. Backporting a
small fix for now.

For testing:
Please follow instructions from this PR description
elastic#146403

(cherry picked from commit ef07c97)
@kertal
Copy link
Member

kertal commented May 31, 2023

@jughosta is working on a quick fix that will bring help to cases when the URL gets very long due to data views with lots of field attributes.

@kertal do you think the quick fix will be sufficient to close this GitHub issue afterwards? I can link the PR as resolving this issue or do you think there is leftover work that should be tracked with this issue?

The PR improves the situation by shortening the alert link. The given cases were triggered by lots of fields with field counts being part of the link. However, it can be still be the case to have very long links. We intend to further improve by migrating persisted data view containing links to no longer be migrated to temporary data views (which contain the spec)

Also with this change, there can be long URL, given users can still use temporary data view.

To completely resolve it we would need to move to short URLs, but this could cause a lot of redundant saved objects, so it would need e.g. functionality to cleanup links with a TTL. However this is a big multi team effort (but on the long run, I think it would be beneficial)

kibanamachine referenced this issue May 31, 2023
# Backport

This will backport the following commits from `main` to `8.8`:
- [[Discover][Alerts] Make alert links shorter
(#158582)](#158582)

<!--- Backport version: 8.9.7 -->

### Questions ?
Please refer to the [Backport tool
documentation](https://github.com/sqren/backport)

<!--BACKPORT [{"author":{"name":"Julia
Rechkunova","email":"julia.rechkunova@elastic.co"},"sourceCommit":{"committedDate":"2023-05-31T05:28:35Z","message":"[Discover][Alerts]
Make alert links shorter (#158582)\n\n- Addresses
https://github.com/elastic/kibana/issues/158262\r\n\r\n##
Summary\r\n\r\nThis PR makes alert links shorter by removing redundant
props from the\r\nencoded state. We should trim it down more in the
future. Backporting a\r\nsmall fix for now.\r\n\r\nFor
testing:\r\nPlease follow instructions from this PR
description\r\nhttps://github.com//pull/146403","sha":"ef07c978689872d2ae3037aa06a0f2f7b23c3582","branchLabelMapping":{"^v8.9.0$":"main","^v(\\d+).(\\d+).\\d+$":"$1.$2"}},"sourcePullRequest":{"labels":["release_note:fix","Team:DataDiscovery","backport:prev-minor","v8.9.0"],"number":158582,"url":"https://github.com/elastic/kibana/pull/158582","mergeCommit":{"message":"[Discover][Alerts]
Make alert links shorter (#158582)\n\n- Addresses
https://github.com/elastic/kibana/issues/158262\r\n\r\n##
Summary\r\n\r\nThis PR makes alert links shorter by removing redundant
props from the\r\nencoded state. We should trim it down more in the
future. Backporting a\r\nsmall fix for now.\r\n\r\nFor
testing:\r\nPlease follow instructions from this PR
description\r\nhttps://github.com//pull/146403","sha":"ef07c978689872d2ae3037aa06a0f2f7b23c3582"}},"sourceBranch":"main","suggestedTargetBranches":[],"targetPullRequestStates":[{"branch":"main","label":"v8.9.0","labelRegex":"^v8.9.0$","isSourceBranch":true,"state":"MERGED","url":"https://github.com/elastic/kibana/pull/158582","number":158582,"mergeCommit":{"message":"[Discover][Alerts]
Make alert links shorter (#158582)\n\n- Addresses
https://github.com/elastic/kibana/issues/158262\r\n\r\n##
Summary\r\n\r\nThis PR makes alert links shorter by removing redundant
props from the\r\nencoded state. We should trim it down more in the
future. Backporting a\r\nsmall fix for now.\r\n\r\nFor
testing:\r\nPlease follow instructions from this PR
description\r\nhttps://github.com//pull/146403","sha":"ef07c978689872d2ae3037aa06a0f2f7b23c3582"}}]}]
BACKPORT-->

Co-authored-by: Julia Rechkunova <julia.rechkunova@elastic.co>
@kertal
Copy link
Member

kertal commented May 31, 2023

Here's another issue to track which will improve the link length : #158729

@pmuellr
Copy link
Member Author

pmuellr commented May 31, 2023

Here's another issue to track which will improve the link length : #158729

Ah, I bet this will save TONs of space, should be great.

We may want to enhance our docs to note that if using temporary data views, the URLs may be too long, and so you should use persistent data views. Or I wonder if we could check that and perhaps provide a different link - like to the rule details or something ...

@kertal
Copy link
Member

kertal commented May 31, 2023

I've opened another issue for a long term solution: #158734

@kertal kertal removed the Team:DataDiscovery Discover, search (e.g. data plugin and KQL), data views, saved searches. For ES|QL, use Team:ES|QL. label Aug 2, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Fixes for quality problems that affect the customer experience Feature:Alerting Team:ResponseOps Label for the ResponseOps team (formerly the Cases and Alerting teams)
Projects
None yet
Development

No branches or pull requests

4 participants