Skip to content

Fix #586: Pass categories as an input for Ad service#600

Merged
puckpuck merged 3 commits into
open-telemetry:mainfrom
madhead:issues/586
Nov 26, 2022
Merged

Fix #586: Pass categories as an input for Ad service#600
puckpuck merged 3 commits into
open-telemetry:mainfrom
madhead:issues/586

Conversation

@madhead

@madhead madhead commented Nov 20, 2022

Copy link
Copy Markdown
Contributor

Fixes #586.

Changes

  1. Pass categories as an input for Ad service
  2. Randomize the ad shown, in case there are multiple ads in a response.
  3. I had to bump the TS target to ES2015 to be able to use Set. I hope this is OK for a demo.

@madhead
madhead requested a review from a team November 20, 2022 04:31
@linux-foundation-easycla

linux-foundation-easycla Bot commented Nov 20, 2022

Copy link
Copy Markdown

CLA Signed

The committers listed above are authorized under a signed CLA.

  • ✅ login: madhead / name: Siarhei (f889af9b1fb046cdee98b3496739f4420f2c3922)

@julianocosta89 julianocosta89 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM

@cartersocha

Copy link
Copy Markdown
Contributor

@madhead please make a changelog entry and I'll merge 🚀

@madhead

madhead commented Nov 21, 2022

Copy link
Copy Markdown
Contributor Author

@cartersocha, done.

@puckpuck puckpuck left a comment

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.

When I run this I get empty app.ads.contextKeys

Screen Shot 2022-11-21 at 11 27 40 PM

@julianocosta89

Copy link
Copy Markdown
Member

When I run this I get empty app.ads.contextKeys

Screen Shot 2022-11-21 at 11 27 40 PM

@puckpuck not all of the requests:
image

@mviitane

Copy link
Copy Markdown
Member

I also tested this and most of the time app.ads.contextKeys is empty. I'd assume this is not how it should be.

@madhead

madhead commented Nov 22, 2022

Copy link
Copy Markdown
Contributor Author

@mviitane , @julianocosta89, @puckpuck

I agree, I saw those empty requests too, but didn't address them here.

I'm not an React expert, but the bigger problem here, IMO, is that AdProvider provides not only ads, but recommendations too, violating single responsibility principle. But wherever it is used, both ads and recommendations are requested.

Looking at three usages of this provider:

  1. <AdProvider productIds={items.map(({ productId }) => productId)}>
    <Layout>
    <S.Cart>
    {(!!items.length && <CartDetail />) || <EmptyCart />}
    <Recommendations />
    </S.Cart>
    <Footer />
    </Layout>
    </AdProvider>

    Cart only uses recommendations.
  2. <AdProvider productIds={[productId, ...items.map(({ productId }) => productId)]}>
    <Layout>
    <S.ProductDetail data-cy={CypressFields.ProductDetail}>
    <S.Container>
    <S.Image $src={picture} data-cy={CypressFields.ProductPicture} />
    <S.Details>
    <S.Name data-cy={CypressFields.ProductName}>{name}</S.Name>
    <S.Description data-cy={CypressFields.ProductDescription}>{description}</S.Description>
    <S.ProductPrice>
    <ProductPrice price={priceUsd} />
    </S.ProductPrice>
    <S.Text>Quantity</S.Text>
    <Select
    data-cy={CypressFields.ProductQuantity}
    onChange={event => setQuantity(+event.target.value)}
    value={quantity}
    >
    {quantityOptions.map(option => (
    <option key={option} value={option}>
    {option}
    </option>
    ))}
    </Select>
    <S.AddToCart data-cy={CypressFields.ProductAddToCart} onClick={onAddItem}>
    <Image src="/icons/Cart.svg" height="15px" width="15px" alt="cart" /> Add To Cart
    </S.AddToCart>
    </S.Details>
    </S.Container>
    <Recommendations />
    </S.ProductDetail>
    <Ad />
    <Footer />
    </Layout>
    </AdProvider>

    Product page uses both ads and recommendations.
  3. <AdProvider productIds={items.map(({ item }) => item?.productId || '')}>
    <Layout>
    <S.Checkout>
    <S.Container>
    <S.Title>Your order is complete!</S.Title>
    <S.Subtitle>We&apos;ve sent you a confirmation email.</S.Subtitle>
    <S.ItemList>
    {items.map(checkoutItem => (
    <CheckoutItem key={checkoutItem.item.productId} checkoutItem={checkoutItem} address={shippingAddress!} />
    ))}
    </S.ItemList>
    <S.ButtonContainer>
    <Link href="/">
    <Button type="submit">Continue Shopping</Button>
    </Link>
    </S.ButtonContainer>
    </S.Container>
    <Recommendations />
    </S.Checkout>
    <Ad />
    <Footer />
    </Layout>
    </AdProvider>

    Order page uses both ads and recommendations.

In some of this contexts the categories are inherently empty.

I would say, this should be fixed and split into two different provider components.

But, I could just "short-circuit" this call, what do you think? Like adding a check, a guard clause, "if the categories are empty, then just return an empty list of adds". Or something similar to remove this unneeded call. A dirty hack, but it should work.

Would it be ok?

@madhead

madhead commented Nov 22, 2022

Copy link
Copy Markdown
Contributor Author

I totally forgot about the load generator. It might be sending incorrect requests too. I've added both fixes, please check the PR now (beware of the cached images!).

@madhead
madhead requested a review from puckpuck November 22, 2022 22:26
@cartersocha

Copy link
Copy Markdown
Contributor

@austinlparker we should test the build time increase from 3 new containers. One thing to keep in mind is that go doesn’t have an operator instrumentation option. We’d need to implement that with the Java service or replace Go

@cartersocha

Copy link
Copy Markdown
Contributor

For the architecture docs and services table my bias would be to label the language Java not Kotlin for the fraud detection service

@julianocosta89

Copy link
Copy Markdown
Member

@cartersocha I think your comments are regarding #457, and not this one😅

@cartersocha

Copy link
Copy Markdown
Contributor

Lol the perils of mobile bulk reviews @julianocosta89. Thanks

@julianocosta89 julianocosta89 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

After the update we always get a context.

@puckpuck puckpuck left a comment

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.

Confirmed this provides proper context keys on all requests types.

@puckpuck
puckpuck merged commit 7f013d8 into open-telemetry:main Nov 26, 2022
jmichalak9 pushed a commit to jmichalak9/opentelemetry-demo that referenced this pull request Mar 22, 2024
…he Ad service (open-telemetry#600)

Co-authored-by: Juliano Costa <[email protected]>
Co-authored-by: Pierre Tessier <[email protected]>
mohamed3637 added a commit to mohamed3637/opentelemetry-demo that referenced this pull request Oct 7, 2025
…he Ad service (open-telemetry#600)

Co-authored-by: Juliano Costa <[email protected]>
Co-authored-by: Pierre Tessier <[email protected]>
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.

Strange adservice behavior. Is it intentional?

5 participants