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

Update Core & add HTTP Otel / Dev UI Port #407

Merged
merged 9 commits into from
Feb 5, 2025
Merged

Update Core & add HTTP Otel / Dev UI Port #407

merged 9 commits into from
Feb 5, 2025

Conversation

Sushisource
Copy link
Member

@Sushisource Sushisource commented Feb 5, 2025

What was changed

  • Added ability to specify UI port in dev server startup
  • Added ability to select HTTP protocol for Otel metrics export

Why?

Checklist

  1. Closes [Feature Request] Allow customization of dev server UI port #396
    closes [Feature Request] OTel metrics over HTTP #392

  2. How was this tested:
    Core tests should be sufficient. Manually verified UI port is set properly.

  3. Any docs updates needed?

@Sushisource Sushisource changed the title [Draft] Core update Update Core & add HTTP Otel / Dev UI Port Feb 5, 2025
@Sushisource Sushisource marked this pull request as ready for review February 5, 2025 02:14
Copy link
Member

@cretz cretz left a comment

Choose a reason for hiding this comment

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

Nice, thanks for knocking out some of my issues while here. Only a couple of minor comments.

src/Temporalio/Bridge/src/testing.rs Outdated Show resolved Hide resolved
Copy link
Member

@cretz cretz Feb 5, 2025

Choose a reason for hiding this comment

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

Not required, but could make this a nested enum inside the OpenTelemetryOptions as just Protocol if you'd like, but here is fine too

Copy link
Member Author

Choose a reason for hiding this comment

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

I stuck with what temporality did

Comment on lines 219 to 220
await EncodeAsync(
codec, cmd.ScheduleNexusOperation.Input).ConfigureAwait(false);
Copy link
Member

@cretz cretz Feb 5, 2025

Choose a reason for hiding this comment

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

Can we surround this with the if-not-null check? Hopefully as of #405 CI will break anyways. (EDIT: it does break in CI, whew)

Copy link
Member Author

Choose a reason for hiding this comment

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

Will do. IMO these should all just check for nullity internally. Feels a bit silly to protect all of them individually.

Copy link
Member

Choose a reason for hiding this comment

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

I'd be ok with that change if you want to make it or open an issue for us to make it later. I think originally I was unnecessarily trying to reduce call counts.

Copy link
Member

@cretz cretz left a comment

Choose a reason for hiding this comment

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

Approving. Don't like the new top-level file, but not a big deal.

@Sushisource Sushisource merged commit 139a853 into main Feb 5, 2025
9 checks passed
@Sushisource Sushisource deleted the core-update branch February 5, 2025 21:36
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.

[Feature Request] Allow customization of dev server UI port [Feature Request] OTel metrics over HTTP
2 participants