Skip to content

feat(shipping): delay international shipments via shippingSlowdown feature flag#3354

Open
tlee768 wants to merge 29 commits into
open-telemetry:mainfrom
tlee768:feat/shipping-slowdown-flag
Open

feat(shipping): delay international shipments via shippingSlowdown feature flag#3354
tlee768 wants to merge 29 commits into
open-telemetry:mainfrom
tlee768:feat/shipping-slowdown-flag

Conversation

@tlee768
Copy link
Copy Markdown

@tlee768 tlee768 commented May 7, 2026

Summary

  • Add shippingSlowdown feature flag (default off) that introduces a 10-second delay for non-US shipping requests when enabled
  • Integrate with flagd via OFREP protocol, with OpenTelemetry tracing on flag evaluations
  • Add FLAGD_HOST and FLAGD_OFREP_PORT env vars and flagd dependency to docker-compose
  • Add comprehensive tests with injectable flag checker and configurable slowdown duration

Closes #346

Test plan

  • cargo test passes in src/shipping
  • docker compose up starts shipping service with flagd dependency
  • Verify no delay for US addresses regardless of flag state
  • Verify 10s delay for non-US addresses when shippingSlowdown flag is enabled
  • Verify no delay for non-US addresses when flag is disabled

Assisted-by: This contribution was prepared with the assistance of an AI development tool.

tlee768 added 6 commits May 7, 2026 12:13
…wn feature flag

Closes open-telemetry#346

Assisted-by: This contribution was prepared with the assistance of an AI development tool.
Assisted-by: This contribution was prepared with the assistance of an AI development tool.
… slowdown duration

Assisted-by: This contribution was prepared with the assistance of an AI development tool.
… slowdown duration

Assisted-by: This contribution was prepared with the assistance of an AI development tool.
…ependency to compose

Assisted-by: This contribution was prepared with the assistance of an AI development tool.
Assisted-by: This contribution was prepared with the assistance of an AI development tool.
@tlee768 tlee768 requested a review from a team as a code owner May 7, 2026 16:21
@github-actions github-actions Bot added docs-update-required Requires documentation update helm-update-required Requires an update to the Helm chart when released labels May 7, 2026
@tlee768 tlee768 marked this pull request as draft May 7, 2026 16:23
tlee768 added 8 commits May 9, 2026 14:19
Assisted-by: This contribution was prepared with the assistance of an AI development tool.
Assisted-by: This contribution was prepared with the assistance of an AI development tool.
Assisted-by: This contribution was prepared with the assistance of an AI development tool.
Assisted-by: This contribution was prepared with the assistance of an AI development tool.
…w OTel API

Assisted-by: This contribution was prepared with the assistance of an AI development tool.
Assisted-by: This contribution was prepared with the assistance of an AI development tool.
Assisted-by: This contribution was prepared with the assistance of an AI development tool.
Assisted-by: This contribution was prepared with the assistance of an AI development tool.
@linux-foundation-easycla
Copy link
Copy Markdown

linux-foundation-easycla Bot commented May 9, 2026

CLA Signed
The committers listed above are authorized under a signed CLA.

@tlee768 tlee768 force-pushed the feat/shipping-slowdown-flag branch 2 times, most recently from 3e754f0 to 9afc649 Compare May 9, 2026 20:51
tlee768 added 3 commits May 9, 2026 20:05
awc::Client is !Send + !Sync (uses Rc internally), so it cannot be stored
in a struct captured by the Send + Sync FlagChecker closure. Instead, store
one Client per actix worker thread using thread_local!, initialized lazily
on first use. Cloning the Client is cheap (bumps an Rc, not the pool).

Assisted-by: This contribution was prepared with the assistance of an AI development tool.
The #[instrument] span was never bridged to OTel since tracing-opentelemetry
is not wired up, so the span and its fields were silently dropped.

Assisted-by: This contribution was prepared with the assistance of an AI development tool.
Assisted-by: This contribution was prepared with the assistance of an AI development tool.
@tlee768 tlee768 force-pushed the feat/shipping-slowdown-flag branch from 9feecdc to 82945d7 Compare May 12, 2026 03:19
@tlee768 tlee768 marked this pull request as ready for review May 12, 2026 03:22
@ps48
Copy link
Copy Markdown
Contributor

ps48 commented May 12, 2026

@tlee768 thanks for the contribution, can you please fix the markdownlint errors:

  • Command to run
  # Markdown lint (requires npm install first)
  make markdownlint

  # Or directly:
  npm install
  npx --no -p markdownlint-cli markdownlint -c .markdownlint.yaml <file>
  • Errors from the CI
 ./src/shipping/README.md:35:16 error MD060/table-column-style Table column style [Table pipe does not align with header for style "aligned"]
./src/shipping/README.md:35:26 error MD060/table-column-style Table column style [Table pipe does not align with header for style "aligned"]
./src/shipping/README.md:35:58 error MD060/table-column-style Table column style [Table pipe does not align with header for style "aligned"]
./src/shipping/README.md:36:31 error MD060/table-column-style Table column style [Table pipe does not align with header for style "aligned"]
./src/shipping/README.md:36:62 error MD060/table-column-style Table column style [Table pipe does not align with header for style "aligned"]

tlee768 added 3 commits May 12, 2026 15:03
Replace closure-based FlagChecker type alias with an enum that supports
generic evaluate::<T>() calls, enabling non-bool flags in the future.

Assisted-by: This contribution was prepared with the assistance of an AI development tool.
…r methods

Assisted-by: This contribution was prepared with the assistance of an AI development tool.
Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Adds a new shippingSlowdown feature flag (default off) to the Rust Shipping service to introduce an artificial delay for international (non‑US) /ship-order requests when enabled, using flagd’s OFREP REST API and emitting tracing around evaluations.

Changes:

  • Introduce a flagd OFREP client and a FlagChecker abstraction to evaluate shippingSlowdown (with a test mock path).
  • Apply a configurable slowdown delay in ship_order for non‑US addresses when the flag is enabled, and add request-path tests.
  • Wire flagd dependency + env vars into docker compose files and add the flag definition to demo.flagd.json.

Reviewed changes

Copilot reviewed 8 out of 8 changed files in this pull request and generated 2 comments.

Show a summary per file
File Description
src/shipping/src/shipping_service/feature_flag.rs Adds a flagd OFREP HTTP client with OTel-instrumented requests and safe fallbacks.
src/shipping/src/shipping_service.rs Adds FlagChecker, injects it + a slowdown duration into ship_order, and adds tests for US/international behavior.
src/shipping/src/main.rs Registers FlagChecker and default slowdown duration into Actix app state.
src/shipping/README.md Documents the new feature flag and related environment variables.
src/flagd/demo.flagd.json Adds the shippingSlowdown flag definition (default variant off).
docker-compose.yml Passes flagd OFREP env vars to shipping and adds flagd to depends_on.
docker-compose.minimal.yml Same as full compose: env var pass-through and flagd dependency for shipping.
CHANGELOG.md Notes the new shipping feature flag integration.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread src/shipping/src/shipping_service.rs Outdated
name = "ShippingSlowdown",
message = "Delaying international shipment due to shippingSlowdown feature flag"
);
actix_web::rt::time::sleep(**slowdown).await;
Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

web::Data<T> wraps an Arc<T>, so Deref for web::Data<Duration> yields Arc<Duration>, not Duration directly. The double deref is correct:

  1. *slowdownArc<Duration> (via web::Data::deref)
  2. **slowdownDuration (via Arc::deref)

This compiles and works as intended.

Comment thread src/shipping/src/shipping_service.rs Outdated
Comment on lines +194 to +200
#[actix_web::test]
async fn test_ship_order_us_flag_on() {
let slowdown = std::time::Duration::from_millis(50);
let checker = FlagChecker::mock().with("shippingSlowdown", true);
let start = std::time::Instant::now();
let order = call_ship_order(Some(make_address("US")), checker, slowdown).await;
assert!(start.elapsed() < slowdown);
Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

The US path does no sleeping — it's just mock flag evaluation + tracking ID generation. 50ms is well above what actix's test harness takes to process a trivial request, so this shouldn't flake in practice. If it does, we can bump the slowdown duration, but I'd rather not add complexity (tokio time mocking, etc.) preemptively.

Copy link
Copy Markdown
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.

Hey @tlee768 thanks for this, and I really want this to happen as a feature flag for the demo. There is an official flagd provider for rust that we should use here instead of a custom client and enum to manage the feature flag evaluation.

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.

Why are we not using the open_feature_flagd provider for Rust instead of creating our own client here? https://docs.rs/open-feature-flagd/latest/open_feature_flagd/

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Done

tlee768 added 5 commits May 14, 2026 21:52
…gd crate

Use FlagdProvider from open-feature-flagd instead of a hand-rolled HTTP
client, removing the FlagdClient wrapper and FlagChecker enum. The
ship_order handler now takes dyn FeatureProvider directly, and tests use
MockFeatureProvider from the open-feature crate.

Assisted-by: This contribution was prepared with the assistance of an AI development tool.
Use the crate's default gRPC resolver (port 8013) instead of REST/OFREP
(port 8016). Update docker-compose files to pass FLAGD_PORT instead of
FLAGD_OFREP_PORT to the shipping service.

Assisted-by: This contribution was prepared with the assistance of an AI development tool.
… up immediately

The default RPC resolver wraps evaluations in an LRU cache with a 60s TTL,
causing demo.flagd.json edits to be invisible until the cache expires.
Setting cache_settings: None bypasses the cache and makes every evaluation
a fresh gRPC unary call, matching the behaviour of the old OFREP client.

Assisted-by: This contribution was prepared with the assistance of an AI development tool.
Narrow open-feature-flagd to the rpc feature only (drops reqwest,
datalogic-rs, notify, semver, etc.). Remove manual FLAGD_HOST/PORT
reads since FlagdOptions::default() already reads them from env.

Assisted-by: This contribution was prepared with the assistance of an AI development tool.
Short-circuit the && so the gRPC call to flagd only happens
when the address is outside the US.

Assisted-by: This contribution was prepared with the assistance of an AI development tool.
@tlee768 tlee768 force-pushed the feat/shipping-slowdown-flag branch from 44e8cde to 3501c1c Compare May 15, 2026 03:36
@tlee768 tlee768 requested a review from puckpuck May 15, 2026 15:13
@puckpuck
Copy link
Copy Markdown
Contributor

Instead of a shippingSlowdown boolean flag, I think a intlShippingSlowdown integer flag with a default value of 0 is a better idea, will keep the code cleaner, and allow for more flexibility with the flag.

Doing this allows us to remove all DEFAULT_SLOWDOWN and the slowdown function parameter, since this is pulled from the featureflag itself now. If the flag doesn't exists we default it to 0 (no sleep).

…lShippingSlowdown flag

The flag value itself now carries the delay in seconds (default 0),
removing the need for DEFAULT_SLOWDOWN and the injected duration parameter.

Assisted-by: This contribution was prepared with the assistance of an AI development tool.
@tlee768
Copy link
Copy Markdown
Author

tlee768 commented May 19, 2026

Instead of a shippingSlowdown boolean flag, I think a intlShippingSlowdown integer flag with a default value of 0 is a better idea, will keep the code cleaner, and allow for more flexibility with the flag.

Doing this allows us to remove all DEFAULT_SLOWDOWN and the slowdown function parameter, since this is pulled from the featureflag itself now. If the flag doesn't exists we default it to 0 (no sleep).

Done

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.

ShippingService should check FeatureFlagService if shipping outside the US takes a while

4 participants