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

Imageprovider #1462

Merged
merged 5 commits into from
Apr 4, 2024
Merged

Imageprovider #1462

merged 5 commits into from
Apr 4, 2024

Conversation

klucsik
Copy link
Contributor

@klucsik klucsik commented Mar 15, 2024

Changes

Added an nginx service to serve the images for the frontend. The service is instrumented with ngx_otel_module.
This adds a new technology demonstration (nginx) and in the future we can introduce more problem patterns (for example adding a sleep module in nginx based on a featureflag to simulate slow loading of images).

Merge Requirements

For new features contributions please make sure you have completed the following
essential items:

  • CHANGELOG.md updated to document new feature additions
  • Appropriate documentation updates in the [docs][]
  • Appropriate Helm chart updates in the [helm-charts][]

@klucsik klucsik force-pushed the imageprovider branch 7 times, most recently from e61dae3 to cb48141 Compare March 15, 2024 10:28
@klucsik klucsik marked this pull request as ready for review March 15, 2024 10:32
@klucsik klucsik requested a review from a team March 15, 2024 10:32
@thiagogcm
Copy link

Although this is valid in demonstrating the nginx-otel capabilities, it does not add much to the overall design IMO.
We already have envoy as an inverse proxy (which we can also use to introduce chaos engineering).

Moreover, instead of nginx, we could rely on Apache as well[1], or many many other providers that already support otel.

[1] https:/open-telemetry/opentelemetry-cpp-contrib/tree/main/instrumentation/otel-webserver-module

@puckpuck puckpuck added the helm-update-required Requires an update to the Helm chart when released label Mar 19, 2024
@klucsik
Copy link
Contributor Author

klucsik commented Mar 20, 2024

Although this is valid in demonstrating the nginx-otel capabilities, it does not add much to the overall design IMO. We already have envoy as an inverse proxy (which we can also use to introduce chaos engineering).

I don't have strong opinion on the design, I think more tech is demonstrated, more value we can provide. I looked into envoys fault injecting and its rather nice indeed.

Moreover, instead of nginx, we could rely on Apache as well[1], or many many other providers that already support otel.

[1] https:/open-telemetry/opentelemetry-cpp-contrib/tree/main/instrumentation/otel-webserver-module

As I see, thisngx_otel_module is the support in nginx ecosystem for otel, so I don't really understand your point, can you provide more context?

@julianocosta89
Copy link
Member

@klucsik I didn't have time to look at it yet (at KubeCon ATM), but I'm just wondering, is this service being called by anyone?
I didn't see changes on the docker-compose so I guess not yet right!?

@klucsik
Copy link
Contributor Author

klucsik commented Mar 21, 2024

Hey, have fun at KubeCON :)
The frontend calls it through the envoy proxy. From browser perspective it's in the same place, but it's not coming from the frontend static, but from nginx

@puckpuck puckpuck added the docs-update-required Requires documentation update label Mar 21, 2024
@julianocosta89
Copy link
Member

@klucsik I've played around with this PR and I'd like to open a discussion here.

Currently this is implemented in a way that frontend-proxy receives a request under http://frontend-proxy:8080/images/products/<IMAGE>.jpg and returns the image.
Nothing is chained to it, so basically we have the following:

frontend-proxy 
      |--> imageprovider

I wonder if instead of it, we should have something like this:

frontend-proxy 
      |--> frontend
              |--> productcatalogservice
      |--> frontend
              |--> imageprovider

or

frontend-proxy 
      |--> frontend
              |--> productcatalogservice
                    |--> imageprovider

In that case, the location of the image would be stored in the product catalog and would be rendered by the frontend.
What do you think? Does that make sense?

I'm also pinging @puckpuck and @austinlparker here to get their opinion on this.

@austinlparker
Copy link
Member

The pattern I'd usually expect for something like this would be that images would be on a CDN (either a subdomain or a separate site) so I'd expect to see a call chain like

proxy -> frontend -> imageprovider

Where the URLs of the images are partials returned by the product catalog that the frontend picks based on env vars or whatever (e.g., when running locally it picks from a local file path, in staging from , in prod from the CDN?)

@klucsik
Copy link
Contributor Author

klucsik commented Mar 27, 2024

I agree with that of a separate site or subdomain would be the most real-world-like setup, but that would add more complexity what we need here in my opinion.
How much we want the local compose deployment and the k8s deployment to differ? I'm not particularly fond of the dev/stg/prod uses different ways to fetch images, but i can accept if we should go in that direction.

I like the url-partials coming from product catalog service, and the frontend is fetching based on that.

@julianocosta89
Copy link
Member

@klucsik I'd go for that:

I like the url-partials coming from product catalog service, and the frontend is fetching based on that.

Then we would have this:

frontend-proxy 
      |--> frontend
              |--> productcatalogservice
      |--> frontend
              |--> imageprovider

@julianocosta89
Copy link
Member

In this case we do not need to worry about different stages and so on.

@klucsik
Copy link
Contributor Author

klucsik commented Mar 28, 2024

the url partial is already coming from the productcatalog service:

string picture = 4;

So the main modification would be that the image goes through the frontendservice server part (the apigateway would be the obvious place for this) instead of just directly going to the imageprovider through the frontendproxy.
That would still produce two disconnected traces if I understand right:

 frontend-proxy 
      |--> frontend
              |--> productcatalogservice

and

  frontend-proxy 
       |--> frontend
               |--> imageprovider

Can we connect them? I suppose we can pass down the span context to the request going to the imageprovider.

I did manage to send arbitrary headers to the image request with some useeffect magic (kudos to @EislM0203):
https:/klucsik/opentelemetry-demo/blob/945480652faf2fcc9998fac52b59e01dd561854e/src/frontend/components/ProductCard/ProductCard.tsx#L60-L70

If we just connect the traces without the apigateway thingie we would still get something like this I suppose:

  frontend-proxy 
       |--> frontend
              |-->  frontend-proxy 
                    |--> imageprovider

would that be suffice?

@julianocosta89
Copy link
Member

That would still produce two disconnected traces if I understand right

I wasn't expecting 2 traces TBH.

Maybe I got lost here, but why do we need the imageprovider to go through the frontendproxy?
Can't the frontend manage that call?

ATM in the PlaceOrder trace we have the checkoutservice doing a bunch of back and forth with currencyservice:

Screenshot 2024-03-28 at 12 55 26

I though that was also a possibility for the frontend. No?

@klucsik
Copy link
Contributor Author

klucsik commented Mar 28, 2024

I'm no nextjs expert, but as I see it now, the imageloading is initiated in the client (aka browser), and its a separate request from the main pageload. I assume we can force it to render every bit on server side, but that would prevent me doing the slowloading problem pattern (images are randomly slowed and visually complete rendering takes a long time), and doesn't seems to be a good idea.
I can experiment with glueing the traces together in the frontend-proxy |--> frontend |--> frontend-proxy |--> imageprovider way, that would require the least amount of work.

@julianocosta89
Copy link
Member

I'm no nextjs expert, but as I see it now, the imageloading is initiated in the client (aka browser), and its a separate request from the main pageload. I assume we can force it to render every bit on server side, but that would prevent me doing the slowloading problem pattern (images are randomly slowed and visually complete rendering takes a long time), and doesn't seems to be a good idea. I can experiment with glueing the traces together in the frontend-proxy |--> frontend |--> frontend-proxy |--> imageprovider way, that would require the least amount of work.

Ah, no need for a hacky solution.

I don't like the approach which we go twice over the frontend-proxy though.

I'd vote for the 1st suggestion here then:
#1462 (comment)

@austinlparker, any thoughts?

@klucsik
Copy link
Contributor Author

klucsik commented Apr 4, 2024

I tested, and adjusted span names and propagation.
image
Everything looks fine, its ready to merge from my end.

@julianocosta89
Copy link
Member

Honestly speaking, I don't understand why would those requests come from the frontend-proxy and not from frontend.

This just looks weird:
image

@klucsik
Copy link
Contributor Author

klucsik commented Apr 4, 2024

It comes from the browser client. What we see there as frontend, is the server part of the frontend.
The images are inserted in the html as plain html img tags. In the product cards the url partial is coming from the product catalog service (in the form of a field in the product object), it gets rendered to a plain img tag, then sent to the browser. Browser finds the image tag, sees the url in it, fetches the image. Doing that it hits the frontend proxy first, and then the image provider. The frontend proxy starts the trace.
Maybe we could tweak the frontend (ideally in a separate PR) to send some trace details, so the frontend page load and image loads would be connected in the traces, but I'm not sure is that a correct usage of traces and how exactly would be possible to implement.

@julianocosta89
Copy link
Member

Also, is this expected?

http.target      //products/OpticalTubeAssembly.jpg

@julianocosta89
Copy link
Member

It comes from the browser client. What we see there as frontend, is the server part of the frontend. The images are inserted in the html as plain html img tags. In the product cards the url partial is coming from the product catalog service (in the form of a field in the product object), it gets rendered to a plain img tag, then sent to the browser. Browser finds the image tag, sees the url in it, fetches the image. Doing that it hits the frontend proxy first, and then the image provider. The frontend proxy starts the trace. Maybe we could tweak the frontend (ideally in a separate PR) to send some trace details, so the frontend page load and image loads would be connected in the traces, but I'm not sure is that a correct usage of traces and how exactly would be possible to implement.

Makes sense, thx for clarifying!

@klucsik
Copy link
Contributor Author

klucsik commented Apr 4, 2024

Oh the leading / should be removed, I'll check it. Nice catch

@klucsik
Copy link
Contributor Author

klucsik commented Apr 4, 2024

Also, is this expected?

http.target      //products/OpticalTubeAssembly.jpg

fixed

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
docs-update-required Requires documentation update helm-update-required Requires an update to the Helm chart when released
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants