Skip to content

feat: [WIP] contribute tower-otel-http-metrics middleware crate #248

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

Draft
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

francoposa
Copy link

@francoposa francoposa commented Apr 16, 2025

Changes

This contribution is a work in progress, just wanted to get it kicked off to have eyes on it.
I was invited to contribute this by @cijothomas, and have received approval from Grafana to contribute work hours towards maintaining this.

Currently the source code lives at https://github.com/francoposa/tower-otel-http-metrics and the crate is at https://crates.io/crates/tower-otel-http-metrics.

There are some various cleanup tasks here - any advice or direction is appreciated!

  • Fixing up the Cargo.toml metadata
  • Moving examples into the common example directory? This might be a bit awkward given the current structure.
  • Code ownership - I am not yet a member of the OpenTelemetry organization as is a requirement for other other contrib repos
  • Change license - it is currently MIT, I am not sure on the specifics of how change it to Apache 2.0 works
  • Changelog, need to get it from the release notes of the current repo
  • ...?

Merge requirement checklist

  • CONTRIBUTING guidelines followed
  • Unit tests added/updated (if applicable)
  • Appropriate CHANGELOG.md files updated for non-trivial, user-facing changes
  • Changes in public API reviewed (if applicable)

Copy link

codecov bot commented Apr 16, 2025

Codecov Report

Attention: Patch coverage is 0% with 155 lines in your changes missing coverage. Please review.

Project coverage is 49.4%. Comparing base (e8a88d9) to head (ea81d0b).

Files with missing lines Patch % Lines
opentelemetry-instrumentation-tower/src/lib.rs 0.0% 155 Missing ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##            main    #248     +/-   ##
=======================================
- Coverage   50.4%   49.4%   -1.1%     
=======================================
  Files         60      61      +1     
  Lines       7629    7784    +155     
=======================================
  Hits        3851    3851             
- Misses      3778    3933    +155     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@francoposa francoposa changed the title [WIP] feat: contribute tower-otel-http-metrics middleware crate feat: [WIP] contribute tower-otel-http-metrics middleware crate Apr 16, 2025
@lalitb
Copy link
Member

lalitb commented Apr 25, 2025

Code ownership - I am not yet a member of the OpenTelemetry organization as is a requirement for other other contrib repos
Change license - it is currently MIT, I am not sure on the specifics of how change it to Apache 2.0 works

I believe above two would be pre-requisite for this PR, rest can be discussed and handled in subsequent PRs. The MIT license is compatible with Apache 2.0, so changing the license should be fine, while keeping the attributing to original:

  • Include the MIT license attribution and copyright notice for the original MIT-licensed code. This can be done in a separate NOTICE file in the root of this component.
This project includes components originally licensed under the MIT License:
Copyright (c) [Year] [Original Author Name]

[Include the full text of the MIT license here if necessary for clarity]
  • The LICENSE file in the root of this component should contain the full text of the Apache 2.0 license, as this will be the primary license for the repository.

Also, if there are any questions specific to license/copyright. more clarity can be obtained by raising the question as issue in the opentelemetry/community repo.

@cijothomas
Copy link
Member

https://github.com/open-telemetry/opentelemetry-rust-contrib/tree/main/opentelemetry-instrumentation-actix-web Consider following structure from this, for examples etc.

@cijothomas
Copy link
Member

  • license attribution and copyright notice for the original MIT-licensed code. This can be done in a separate NOTICE file in the root of this component.

Could this be considered as normal code contribution? Or this is to be treated as a code-donation? @lalitb do you know ?

@francoposa
Copy link
Author

LICENSE and NOTICE updated in the form @lalitb suggested.
Package name updated (not sure how this would affect the current crate listing?), and references to the original github repo have been converted.

@cijothomas Re:

https://github.com/open-telemetry/opentelemetry-rust-contrib/tree/main/opentelemetry-instrumentation-actix-web Consider following structure from this

I don't see much structure to follow there considering how much more is in the examples for this package - what I have for examples is a much larger and comprehensive superset of what's in the actix-web directory.


#[tokio::main]
async fn main() {
let exporter = opentelemetry_otlp::MetricExporter::builder()
Copy link
Member

Choose a reason for hiding this comment

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

for instrumentation's docs, it's easy to use the stdout exporter. It'll be hard to maintain proper OTLP example for each instrumentations.

@@ -0,0 +1,21 @@
apiVersion: 1
Copy link
Member

Choose a reason for hiding this comment

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

can you add a readme.md file to the development folder explaining how to leverage the docker-compose, and the grafana dashboards?

I am also not sure how much detailed example we want to maintain per instrumentation. it may makes sense to move some of these to common examples, as they are applicable to any web apps...

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.

3 participants