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

[frontend] [ops golf?] Pass collector URL to frontend via reverse proxy. #508

Merged
merged 18 commits into from
Oct 21, 2022
Merged

[frontend] [ops golf?] Pass collector URL to frontend via reverse proxy. #508

merged 18 commits into from
Oct 21, 2022

Conversation

austinlparker
Copy link
Member

This fixes the current problem of the collector URL not being passed into the frontend, as Next.JS will not pass env vars into the client except ones known at build time.

It accomplishes this by way of establishing a reverse proxy that serves the Next.JS app (best practice!) and uses it to set a cookie that the client code reads on load.

@austinlparker austinlparker requested a review from a team October 21, 2022 03:22
@austinlparker
Copy link
Member Author

@puckpuck had an excellent idea to use envoy rather than nginx. there is one small catch here, that we have to build an envoy container in order to get envsubst but that's probably not the end of the world.

Someone more enterprising than I can enable tracing/metrics off this post v1.

Copy link
Contributor

@puckpuck puckpuck left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a few small naming things.

src/frontendProxy -> src/frontend proxy all lowercase. This is to stay consistent with the other services

docker-compose.yml Outdated Show resolved Hide resolved
@cartersocha
Copy link
Contributor

The frontend is broken on my machine? Is port 8080 still the right connection or do I need to navigate to the reverse proxy?

Copy link
Contributor

@cartersocha cartersocha left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Small pause to prevent merging before localhost question is answered

@cartersocha
Copy link
Contributor

If we update the docker file to port 80 we should be good

@cartersocha
Copy link
Contributor

cartersocha commented Oct 21, 2022

and changelog + architecture

@austinlparker
Copy link
Member Author

If we update the docker file to port 80 we should be good

what part of the dockerfile? the compose file or the frontend?

@puckpuck
Copy link
Contributor

We need to update the readmes to indicate we run on port 80 now

@austinlparker
Copy link
Member Author

Updated the docker readme with the current state

@puckpuck
Copy link
Contributor

Not for this PR, but it would be nice if there is a way to force mermaid to not draw a box in the top right corner that gets covered by the github icons
Screen Shot 2022-10-21 at 1 59 39 PM

@cartersocha cartersocha dismissed their stale review October 21, 2022 18:51

Because I am myself

@austinlparker austinlparker merged commit 7a499f8 into open-telemetry:main Oct 21, 2022
@austinlparker austinlparker deleted the passCollectorToFE branch October 21, 2022 18:51
jmichalak9 pushed a commit to jmichalak9/opentelemetry-demo that referenced this pull request Mar 22, 2024
…xy. (open-telemetry#508)

* support setting collector URL for browser

* rename to frontend-proxy

* refactor to use envoy rather than nginx

* cleanup

* fix composefile

* delete unneeded nginx conf template

* cleanup naming

* set cors to *

* also front ffs with envoy

* update docs, changelog, etc

* update one more port mapping in docs

* update for envoy url

* update for envoy url

* Revert "update for envoy url"

This reverts commit 63c7bcf.

* fixup

Co-authored-by: Pierre Tessier <pierre@pierretessier.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants