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

Ensure random ads code branch is exercised by the load generator #656

Merged

Conversation

jlawrienyt
Copy link

@jlawrienyt jlawrienyt commented Dec 22, 2022

Changes

  • Add a new product in a new category, "Books"
  • Update load generator script to fetch the new product, as well as request ads in the new book category, and request ads without any category, in order to test the random ad delivery code path
  • This is in preparation for adding custom business metrics to the ad service.

Merge Requirements

For new features contributions please make sure you have completed the following
essential items:

  • CHANGELOG.md updated to document new feature additions
  • Appropriate documentation updates in the docs folder

Maintainers will not merge until the above have been completed. If you're unsure
which docs need to be changed ping the
@open-telemetry/demo-approvers.

@jlawrienyt jlawrienyt requested a review from a team December 22, 2022 18:43
@fatsheep9146
Copy link
Contributor

Hi I wonder if 'This is in preparation for #73.', what metric do you want to provide if this pr is merged?

@jlawrienyt jlawrienyt force-pushed the adservice.test-random-ads branch from 085abb4 to cd7762d Compare December 23, 2022 14:27
@jlawrienyt
Copy link
Author

Hi I wonder if 'This is in preparation for #73.', what metric do you want to provide if this pr is merged?

I was thinking of an ad requests counter with two dimensions: request type of either targeted or "not targeted", and response type of either targeted or random.

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.

Do we have permission to use this image from National Geographic?

@puckpuck
Copy link
Contributor

Maybe it was mentioned in a SIG meeting that I missed, but can we get a detailed plan in the #73 issue? I'm ok with adding another product and category, but if this is in reference to that issue, I would like to understand what the bigger picture looks like.

@github-actions
Copy link

github-actions bot commented Jan 1, 2023

This PR was marked stale due to lack of activity. It will be closed in 7 days.

@jlawrienyt
Copy link
Author

Do we have permission to use this image from National Geographic?

Hmm, good point. Any tips on where to get images where licensing won't be an issue? Alternatively, we don't strictly need to add a new product in order have the load generator exercise the random ads code branch since the load generator is hitting the ads API directly.

@jlawrienyt
Copy link
Author

Maybe it was mentioned in a SIG meeting that I missed, but can we get a detailed plan in the #73 issue? I'm ok with adding another product and category, but if this is in reference to that issue, I would like to understand what the bigger picture looks like.

Sorry, I didn't realize this would be appropriate fodder for the SIG meeting. I'm happy to bring it up at the next meeting.

@jlawrienyt jlawrienyt force-pushed the adservice.test-random-ads branch 2 times, most recently from dbe3f87 to 032ec19 Compare January 3, 2023 19:03
@jlawrienyt
Copy link
Author

Do we have permission to use this image from National Geographic?

Hmm, good point. Any tips on where to get images where licensing won't be an issue? Alternatively, we don't strictly need to add a new product in order have the load generator exercise the random ads code branch since the load generator is hitting the ads API directly.

I've updated the new product to use a creative common licensed image.

@puckpuck
Copy link
Contributor

puckpuck commented Jan 3, 2023

Sorry, I didn't realize this would be appropriate fodder for the SIG meeting. I'm happy to bring it up at the next meeting.

Providing color in issue #73 itself would be fine as well. I just want to understand the big picture.

@jlawrienyt
Copy link
Author

Sorry, I didn't realize this would be appropriate fodder for the SIG meeting. I'm happy to bring it up at the next meeting.

Providing color in issue #73 itself would be fine as well. I just want to understand the big picture.

I've added a couple of paragraphs describing my general proposal in #73, let me know what you think.

@jlawrienyt jlawrienyt force-pushed the adservice.test-random-ads branch from 6cd8c9a to 2ec278e Compare January 9, 2023 15:06
@cartersocha
Copy link
Contributor

to be merged

Copy link
Contributor

@cartersocha cartersocha left a comment

Choose a reason for hiding this comment

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

Lgtm

@julianocosta89 julianocosta89 merged commit e427c9a into open-telemetry:main Jan 10, 2023
@jlawrienyt jlawrienyt deleted the adservice.test-random-ads branch January 12, 2023 15:55
jmichalak9 pushed a commit to jmichalak9/opentelemetry-demo that referenced this pull request Mar 22, 2024
…n-telemetry#656)

* Ensure random ads code branch is exercised by the load generator

* Switching to new product with image under CC license

Co-authored-by: Juliano Costa <julianocosta89@outlook.com>
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.

6 participants