-
Notifications
You must be signed in to change notification settings - Fork 4
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
tweaks in service to containerization #21
Conversation
* update deps * conform the service name to the pattern of the other services * turn on W3C trace header propagation
Don't need to explicitly set a batch span processor piped to an OTLP exporter; that's the default. Instead, tell the SDK "configure thyself" in code and set environment variables appropriately.[1] [1] https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/protocol/exporter.md
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.
Mostly just questions. Seems cromulent enough. 👍
var ( | ||
apiKey = os.Getenv("HONEYCOMB_API_KEY") | ||
dataset = os.Getenv("HONEYCOMB_DATASET") | ||
nameServiceUrl = os.Getenv("NAME_ENDPOINT") + "/name" |
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.
Do these have defaults set somewhere (a la the Python code)?
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.
Nnnnno. You're right; they should get some!
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.
Trying to update this has turned into a yak shave and I don't know why.
@@ -6,4 +6,4 @@ RUN bundle install | |||
COPY frontend.ru /myapp | |||
|
|||
EXPOSE 7000 | |||
CMD [ "rackup", "frontend.ru", "--server", "puma", "--host", "0.0.0.0"] | |||
CMD [ "bundle", "exec", "rackup", "frontend.ru", "--server", "puma", "--host", "0.0.0.0"] |
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.
Oh, I wondered about this in my recent Dockerfile. I couldn't recall if the rackup
binary would just be whatever Bundler installed, thus making bundle exec
unnecessary to execute the command. My image worked, so I guess that's the case...?
But I suppose one side effect is that bundle exec
requires the Gemfile gems, which means you wouldn't need to require
things manually in frontend.ru
, which might be something worth going for.
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.
It should work without bundle exec
. This might be cruft left from when I was doing some shenanigans with pulling in an alternative version of OpenTelemetry Ruby checked out to the vendor/
subdirectory. To have that directory included in the Ruby load path, I had to bundle exec
.
A variety of smallish changes to allow these processes to run in containers and inject some config via environment variables.