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

Flux builder doesn't show results in firefox #4030

Closed
rbetts opened this issue Jul 26, 2018 · 6 comments
Closed

Flux builder doesn't show results in firefox #4030

rbetts opened this issue Jul 26, 2018 · 6 comments
Assignees
Labels
epic/flux Issues for 1.7 flux release kind/bug
Milestone

Comments

@rbetts
Copy link

rbetts commented Jul 26, 2018

Using nightlies from July 26th. Verified to work correctly in Chrome. Screenshots attached showing that csv results where received .. and some javascript errors that seem related.

screen shot 2018-07-26 at 14 15 16

screen shot 2018-07-26 at 14 14 48

@chnn
Copy link
Contributor

chnn commented Jul 26, 2018

For whoever picks this up, the runtime error originates here:

const reader = resp.body.getReader()

The issue is that resp (a Response object) doesn't have a body property in Firefox (it's currently behind a feature flag in Firefox).

We need some other way of reading partial responses from the Flux proxy. We can't read the entire response and then truncate it, since the response could be huge (gigabytes). As far as I know, the Body.getReader() method is the only way of achieving this browser side. I propose we move the logic to the server side handler.

@jaredscheib jaredscheib self-assigned this Jul 27, 2018
@jaredscheib jaredscheib added this to the 1.7.0 milestone Jul 30, 2018
@jaredscheib
Copy link
Contributor

After spiking on this, TLDR of my findings is that I agree with @chnn that this work should be done on the backend, until the Streams API is sufficiently implemented in all browser vendors that we wish to support.

One solution could be something like a paging API that clients would consume. This would make this feature consistent and uniform across all browser vendors in the UI/Chronograf, as well as for other consumers of the platform API going forward.

Moving this work to the server would require a refactor of the current code by which we consume responses to Flux queries from the UI/Chronograf, as it consumes the Response.Body.body as a ReadableStream, into one that simply pages through server results at a desirable rate and limit. This is part of the aforementioned Streams API that is not yet sufficiently implemented in all browser vendors.

Non-TLDR:

Response.Body is not yet implemented in Firefox, and the timeline is unclear as to when it will be: https://bugzilla.mozilla.org/show_bug.cgi?id=1147061. I tried to look into a "polyfill"-type approach, where we could extend the class of Body to include a body property, but didn't find this approach nor this solution anywhere on the interwebs. The closest I found were https://github.com/moll/js-fetch-jsonify and https://polyfill.io/v2/docs/features/, and neither of these is relevant. Maybe I'm not thinking about this problem the right way, but my intuition tells me currently that extending the Body class is a fraught, risky, and time-consuming approach that would not at all be worth it, as it's probably better solved in the server anyway.

The essential reason for not trying to solve this in the client is that various browser vendors have varying and inconsistent support for streaming, whether via implementation of the W3C ratified Streams API spec (https://www.w3.org/TR/streams-api/) or via the XHR (XMLHttpRequest) API.

The Streams API has been implemented in Chrome and Edge fully, it has not been fully implemented in Firefox or Internet Explorer, and unknown in Safari: https://developer.mozilla.org/en-US/docs/Web/API/Streams_API. The issue at hand here is a case in point, where Response.Body.body has not been fully implemented in Firefox: see the compatibility table at the body row at https://developer.mozilla.org/en-US/docs/Web/API/Body.

XHR is implemented in all browsers, but wasn't really built for streaming: https://hpbn.co/xmlhttprequest/. Here's how XHR can be used for "streaming", but it doesn't work in all browsers: https://gist.github.com/igrigorik/5736866. While some vendors engineered a way to make it work for "streaming" (what is known as "chunked transfer"), Firefox and Internet Explorer both would require their own custom responseType: https://developer.mozilla.org/en-US/docs/Web/API/XMLHttpRequest/responseType. However, in Oct 2017, Firefox deprecated two of the three such moz-prefixed responseTypes, and the remaining one is on the way out: https://www.fxsitecompat.com/en-CA/docs/2017/prefixed-xmlhttprequest-response-types-including-moz-blob-are-no-longer-supported/ and https://bugzilla.mozilla.org/show_bug.cgi?id=1120171. So this could either not be possible to be made to work in Firefox at all with the one remaining moz-prefixed response type, or it could be flaky; and, it would be a fragile solution at best, and would require rework relatively soon probably, given: 1) those other recent deprecations, 2) Response.Body is on the horizon; 3) the Streams API is on the horizon.

Moreover, as for user-agent detection in case we even wanted to try to solve this problem with any of the XHR line of inquiry above, we'd have to detect or infer the client somehow to get it right per browser: https://developer.mozilla.org/en-US/docs/Learn/Tools_and_testing/Cross_browser_testing/Feature_detection. I was able to detect that Body.body is not implemented via the following, but this wouldn't help us know which user-agent was our client:

if ('body' in window.Response.prototype)
} else {
}

User-agent sniffing is discouraged for being notoriously fickle and inaccurate.

Due to the limitations of XHR, our AJAX library axios also does not solve this problem: axios/axios#479.

So this is all to say, let's solve this in the server until the Streams API is implemented sufficiently in all browser vendors that we wish to support.

@jaredscheib jaredscheib removed their assignment Aug 2, 2018
@jaredscheib
Copy link
Contributor

Blocked on stabilization of platform so that we can solve this on the server side.

@lukevmorris
Copy link
Contributor

lukevmorris commented Aug 2, 2018

From Slack discussion between me + @jaredscheib :

  • might also be worth considering using flux's limit() function to cap the amount of data returned
  • problem seems to be that
    • chronograf requests some data
    • server delivers a response of infinite size
    • frontend needs to avoid processing infinite data
  • lazily processing the data on the frontend and stopping when reaching a maximum seems like a hacky solution to begin with
  • core problem is the "infinite size"
  • so i agree that "moving it to the server" is the right approach
  • but having the server proxy lazily process the data and stopping when reaching a maximum size feels like we're just kicking the can down the road
  • maybe best to ask the storage tier to return a maximum response size

@jaredscheib
Copy link
Contributor

Thanks, @lukevmorris!

I did forget to mention that we'd discussed adding limit() – it's a good idea. We'd need to think through the UX on this, as users may be confused as to why this is happening.

Whether via limit() or just in general, I think we may need to carefully think through the UX on how to handle response body sizes of the time series data. It would be great if the server/database can tell us how much data we can expect, and show that to the user somehow, and/or to somehow transparently include users such a data-limiting experience. Something to think about both for query efficiency and good UX.

@russorat russorat added the epic/flux Issues for 1.7 flux release label Sep 11, 2018
@russorat
Copy link
Contributor

russorat commented Oct 1, 2018

let's set up a meeting to discuss options.

@russorat russorat self-assigned this Oct 22, 2018
@chnn chnn self-assigned this Oct 23, 2018
This was referenced Oct 24, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
epic/flux Issues for 1.7 flux release kind/bug
Projects
None yet
Development

No branches or pull requests

5 participants