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

feat: Feature/opentel #1770

Closed
wants to merge 27 commits into from
Closed

Conversation

bearrito
Copy link
Contributor

@bearrito bearrito commented Apr 19, 2024

Description

Adds in support for Open Telemetry as an exporter

  • The exporter exports to opentel collectors. The exporter can natively handle any of the supported opentel exporters. However, configuration of these exporters isn't supported yet. If users wanted them I think it's feasible, it just makes configuration via relay difficult.
  • The reflection code to convert interfaces to opentel attributes is sketchy but tested. If there is a better approach I can do that.
  • Opentel really wants you to store providers as global, I think what I have is resource safe, but it's worth noting how the provider works.
  • I'd like to add more serious testing with the testcontainer

Changes include

  • [+] New feature (non-breaking change that adds functionality)

Closes issue(s)

#1375

Checklist

  • [+] I have tested this code
  • [+] I have added unit test to cover this code
  • I have updated the documentation (README.md and /website/docs)
  • I have followed the contributing guide

Copy link

netlify bot commented Apr 19, 2024

Deploy Preview for go-feature-flag-doc-preview failed.

Name Link
🔨 Latest commit db5fbdc
🔍 Latest deploy log https://app.netlify.com/sites/go-feature-flag-doc-preview/deploys/66352ec0c2616000082ee58a

@bearrito
Copy link
Contributor Author

bearrito commented Apr 19, 2024

@thomaspoignant Can you let me know what you think of this?

Since the Value on FeatureEvent is an interface it was a bit tricky to get in as a set of attributes. I had to make some design decisions on how to do that.

Copy link

codecov bot commented Apr 22, 2024

Codecov Report

Attention: Patch coverage is 76.92308% with 42 lines in your changes missing coverage. Please review.

Project coverage is 86.21%. Comparing base (32e4fc0) to head (db5fbdc).
Report is 782 commits behind head on main.

Files with missing lines Patch % Lines
exporter/opentelemetryexporter/exporter.go 76.53% 31 Missing and 11 partials ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1770      +/-   ##
==========================================
- Coverage   86.72%   86.21%   -0.51%     
==========================================
  Files          89       90       +1     
  Lines        3367     3548     +181     
==========================================
+ Hits         2920     3059     +139     
- Misses        345      376      +31     
- Partials      102      113      +11     

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

@bearrito bearrito changed the title [DRAFT]Feature/opentel feat:Feature/opentel Apr 22, 2024
@thomaspoignant thomaspoignant changed the title feat:Feature/opentel feat: Feature/opentel Apr 29, 2024
@thomaspoignant
Copy link
Owner

I am sorry to be slow on the review. I will do it very soon.

@bearrito
Copy link
Contributor Author

bearrito commented May 1, 2024

I am sorry to be slow on the review. I will do it very soon.

No problem. Life exists outside github!

Copy link

sonarqubecloud bot commented May 3, 2024

Quality Gate Passed Quality Gate passed

Issues
1 New issue
0 Accepted issues

Measures
0 Security Hotspots
No data about Coverage
0.0% Duplication on New Code

See analysis details on SonarCloud

@bearrito
Copy link
Contributor Author

@thomaspoignant Anything else on this? I'm going on holiday and am closing out my open pr's

@thomaspoignant
Copy link
Owner

Sorry I had no time to test it properly.
But don't worry we can look at it after your vacation.

@thomaspoignant thomaspoignant force-pushed the main branch 3 times, most recently from 1021190 to cc7c8d4 Compare July 25, 2024 15:40
@github-actions github-actions bot added the Stale When an issue is open for too long label Oct 25, 2024
@github-actions github-actions bot removed the Stale When an issue is open for too long label Oct 25, 2024
@thomaspoignant
Copy link
Owner

I am sorry this issue has been stale for super long time and I have decided to rework this part from scratch.
I will take inspiration from your PR to open a new one pretty soon.

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.

2 participants