Skip to content

fix[batch-submitter]: read metrics env vars correctly#890

Merged
annieke merged 2 commits intodevelopfrom
annie/fix-bs-env-vars
May 17, 2021
Merged

fix[batch-submitter]: read metrics env vars correctly#890
annieke merged 2 commits intodevelopfrom
annie/fix-bs-env-vars

Conversation

@annieke
Copy link
Copy Markdown
Contributor

@annieke annieke commented May 17, 2021

Description
Fixes reading the default prom server env vars before we switch over to fully using bcfg.

Additional context
unblocks @optimisticben !

Metadata

  • Fixes #[Link to Issue]

@annieke annieke requested a review from karlfloersch as a code owner May 17, 2021 16:30
@changeset-bot
Copy link
Copy Markdown

changeset-bot bot commented May 17, 2021

⚠️ No Changeset found

Latest commit: fae9a05

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

@annieke annieke requested a review from optimisticben May 17, 2021 16:31
@tynes
Copy link
Copy Markdown
Contributor

tynes commented May 17, 2021

Do you mind adding a changeset so that we can publish a new release including this?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Can we rename prometheus-port -> metrics-port and prometheus-hostname -> metrics-address?

Should these env name be separated by package? Something like BATCHSUBMITTER_METRICS_PORT and BATCHSUBMITTER_METRICS_ADDRESS?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

The prefix BATCH_SUBMITTER is supported based on this line here:

const config: Bcfg = new Config('batch-submitter')
. Whatever name is passed to the config object will parse out prefixed env vars where the name is the prefix

A library called bcfg is used for env var parsing. It is lightweight, simple and has no dependencies besides the JS std library. It parses prefixed env vars, CLI args and can read from a config file at ~$HOME/.<name-passed-to-config>/* with the config.open function - https://github.com/bcoin-org/bcfg/blob/1216c775832c2e138ae6451912a27ba496dc4386/lib/config.js#L128

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Oh, nice! LGTM then

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This still doesn't address the change to metrics-*

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

^renamed!

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Oh, nice! LGTM then

@annieke annieke force-pushed the annie/fix-bs-env-vars branch from 172a8bd to fae9a05 Compare May 17, 2021 18:36
@annieke annieke merged commit 9fea7c9 into develop May 17, 2021
@annieke annieke deleted the annie/fix-bs-env-vars branch May 17, 2021 19:01
InoMurko pushed a commit to omgnetwork/optimism that referenced this pull request May 25, 2021
…ism#890)

* fix[batch-submitter]: read metrics env vars correctly

* rename to metrics
theochap pushed a commit that referenced this pull request Dec 10, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

A-op-batcher Area: op-batcher

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants