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

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

Merged
merged 3 commits into from
Nov 26, 2022

Conversation

madhead
Copy link
Contributor

@madhead madhead commented Nov 20, 2022

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

linux-foundation-easycla bot commented Nov 20, 2022

CLA Signed

The committers listed above are authorized under a signed CLA.

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

Copy link
Member

@julianocosta89 julianocosta89 left a comment

Choose a reason for hiding this comment

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

LGTM

@cartersocha
Copy link
Contributor

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

@madhead
Copy link
Contributor Author

madhead commented Nov 21, 2022

@cartersocha, done.

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

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

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

@julianocosta89
Copy link
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
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
Copy link
Contributor Author

madhead commented Nov 22, 2022

@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
Copy link
Contributor Author

madhead commented Nov 22, 2022

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
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
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
Member

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

@cartersocha
Copy link
Contributor

Lol the perils of mobile bulk reviews @julianocosta89. Thanks

Copy link
Member

@julianocosta89 julianocosta89 left a comment

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.

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

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]>
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