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

Add ads served counter metric with request and response types #678

Merged
merged 2 commits into from
Jan 15, 2023

Conversation

jlawrienyt
Copy link

@jlawrienyt jlawrienyt commented Jan 9, 2023

Changes

Add a new counter to track the number of ad requests along with whether the request is targeted or not with context keys, and whether or not the response is targeted or random. This fulfills some of #73.

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 January 9, 2023 16:11
@jlawrienyt jlawrienyt force-pushed the adservice.add-metrics branch 3 times, most recently from b400aba to e9fd555 Compare January 9, 2023 17:00
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, I would just like to test it out whenever #656 gets merged.

@jlawrienyt
Copy link
Author

I had some questions on some of the requirements in #73. Can post in slack also if that'd be the more appropriate place.

  1. Services should use push metrics where possible. Is this just using the OTLP exporter for metrics?
  2. Collectors should perform metric transforms to normalize resource and service metrics. Not sure what this was driving at

@julianocosta89
Copy link
Member

julianocosta89 commented Jan 10, 2023

@cartersocha may have some insights about it: #678 (comment)

@cartersocha
Copy link
Contributor

The original requirements were created when metrics weren’t widely supported and mostly based off the spec. The requirements are probably worth revisiting in general.

In my mind we’d like at least multiple manual instrument types, trace exemplars, views, instrumentation libraries, regular attribute enrichment, and resource attribute enrichment.

What do y’all think?

I think the collector service / resource line poorly refers to resource enrichment haha.

@jlawrienyt
Copy link
Author

In my mind we’d like at least multiple manual instrument types, trace exemplars, views, instrumentation libraries, regular attribute enrichment, and resource attribute enrichment.

Just to clarify a few things: by instrumentation libraries do you mean auto-instrumentation libraries providing metrics? And for resource attribute enrichment, I believe resource attributes can only be set when initializing the sdk, so do you mean adding more resource attributes beyond what might be auto-configured (also that would seem to apply beyond metrics, to all signal types)?

Otherwise, I think the list looks good.

@cartersocha
Copy link
Contributor

Yes those. Auto instrumentation is a fraught topic given all the flavors and can mean many things. There's agent or injection based auto instrumentation like in Python-Java / Dotnet and general instrumentation libraries.

We probably want to separate those as distinct features in the feature matrix besides just saying instrumentation libraries?

Agent based metrics / traces vs. instrumentation libraries added to a provider?

Resource detectors like severin has been adding. They need to be added to both providers (if available)

@julianocosta89
Copy link
Member

I like it!
image

@jlawrienyt
Copy link
Author

Yes those. Auto instrumentation is a fraught topic given all the flavors and can mean many things. There's agent or injection based auto instrumentation like in Python-Java / Dotnet and general instrumentation libraries.

We probably want to separate those as distinct features in the feature matrix besides just saying instrumentation libraries?

Agent based metrics / traces vs. instrumentation libraries added to a provider?

Maybe the distinction between auto vs manual for instrumentation libraries is not so important, at least in terms of the feature matrix.

BTW, looks like your last PR overwrite the metrics table with trace info; going to revert that in this PR and attempt to update the columns to capture this dicussion.

Update documentation
Configure collector's prometheus exporter to convert resource attributes to prometheus labels
@jlawrienyt jlawrienyt force-pushed the adservice.add-metrics branch from a3d6d83 to c606777 Compare January 13, 2023 16:02
@cartersocha
Copy link
Contributor

thanks for catching that @jlawrienyt ! lgtm after resolving the conflict and thanks for the contribution 🚀

@julianocosta89 julianocosta89 merged commit a1ffa72 into open-telemetry:main Jan 15, 2023
@jlawrienyt jlawrienyt deleted the adservice.add-metrics branch January 17, 2023 14:52
jmichalak9 pushed a commit to jmichalak9/opentelemetry-demo that referenced this pull request Mar 22, 2024
…elemetry#678)

Update documentation
Configure collector's prometheus exporter to convert resource attributes to prometheus labels

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.

4 participants