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: custom attributes are ignored - #5084 #5129

Merged
merged 3 commits into from
May 29, 2024

Conversation

zailic
Copy link
Contributor

@zailic zailic commented Feb 18, 2024

OK... i opened a new PR for #5084 issue, as i shot myself in a foot trying to modify a previous commit that I pushed under another github user.

Copy link

codecov bot commented Feb 19, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 63.5%. Comparing base (2ca9fcb) to head (3b158db).

Additional details and impacted files

Impacted file tree graph

@@          Coverage Diff          @@
##            main   #5129   +/-   ##
=====================================
  Coverage   63.5%   63.5%           
=====================================
  Files        194     194           
  Lines      12059   12061    +2     
=====================================
+ Hits        7663    7667    +4     
+ Misses      4179    4178    -1     
+ Partials     217     216    -1     
Files Coverage Δ
instrumentation/net/http/otelhttp/handler.go 91.9% <100.0%> (+<0.1%) ⬆️
instrumentation/net/http/otelhttp/labeler.go 100.0% <100.0%> (+11.7%) ⬆️
instrumentation/net/http/otelhttp/transport.go 97.4% <100.0%> (+<0.1%) ⬆️

@zailic
Copy link
Contributor Author

zailic commented Mar 26, 2024

@dmathieu - Could you please let me know if there's any changes or information from me in order to move forward with this PR?

CHANGELOG.md Outdated Show resolved Hide resolved
instrumentation/net/http/otelhttp/labeler.go Show resolved Hide resolved
@zailic
Copy link
Contributor Author

zailic commented Apr 23, 2024

Hi @dmathieu , when you have a moment, could you please take a look at this PR? Thank you!

@zailic zailic requested a review from dmathieu April 23, 2024 19:09
@dmathieu
Copy link
Member

There is still a lint issue.

@zailic
Copy link
Contributor Author

zailic commented Apr 25, 2024

There is still a lint issue.

@dmathieu Merged the latest main branch changes and did a make precommit on my local...I hope this will solve the failing build step issue.

@dmathieu
Copy link
Member

The issue appears to be that go.sum is changed by the linter. You may have to run go mod tidy

@zailic
Copy link
Contributor Author

zailic commented Apr 26, 2024

The issue appears to be that go.sum is changed by the linter. You may have to run go mod tidy

it's strange i did a make precommit(which incluedes a go mod tidy step) before the push...

@zailic
Copy link
Contributor Author

zailic commented Apr 26, 2024

The issue appears to be that go.sum is changed by the linter. You may have to run go mod tidy
@dmathieu - i pushed the modified go.sum - could you please give it another try?

@zailic zailic requested a review from dmathieu May 2, 2024 15:33
Copy link

linux-foundation-easycla bot commented May 20, 2024

CLA Signed

The committers listed above are authorized under a signed CLA.

  • ✅ login: zailic / name: Ionut Ovidiu Zailic (0c3d401)
  • ✅ login: MrAlias / name: Tyler Yahn (3b158db)
  • ✅ login: dmathieu / name: Damien Mathieu (852f3ed)

@dmathieu
Copy link
Member

Something is wrong with your merges.

@zailic
Copy link
Contributor Author

zailic commented May 21, 2024

Something is wrong with your merges.

strange... i only merged latest main branch changes without having any conflicts

@zailic zailic requested a review from dmathieu May 21, 2024 12:10
@zailic
Copy link
Contributor Author

zailic commented May 21, 2024

@dmathieu - should I wait for more reviewers, to get this merged? It's a little bit painful to keep this branch up to date. Thanks.

@dmathieu
Copy link
Member

PRs need at least 2 reviewers, and can only be merged by a maintainer. So yes, more reviews are required.
Also, while the diff of your PR looks good, there are a lot of commits attached to it, which cause a CLA issue. It won't be mergeable that way.

zailic added a commit to zailic/opentelemetry-go-contrib that referenced this pull request May 21, 2024
@zailic
Copy link
Contributor Author

zailic commented May 21, 2024

PRs need at least 2 reviewers, and can only be merged by a maintainer. So yes, more reviews are required. Also, while the diff of your PR looks good, there are a lot of commits attached to it, which cause a CLA issue. It won't be mergeable that way.

Understood. Thanks! I've squashed bloating commits.

@zailic
Copy link
Contributor Author

zailic commented May 21, 2024

Hi @Aneurysm9,

I hope you're doing well. I would appreciate your review when you have a moment.

Thanks!

@MrAlias MrAlias merged commit bd97adf into open-telemetry:main May 29, 2024
23 checks passed
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.

None yet

4 participants