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

Jump to bottom

[GO]: OTel API operation naming: Unskipped / Fixed the tests in accordance to OTel attributes remapping #1804

Merged
merged 21 commits into from Nov 14, 2023
Merged

[GO]: OTel API operation naming: Unskipped / Fixed the tests in accordance to OTel attributes remapping #1804

merged 21 commits into from Nov 14, 2023

Conversation

dianashevchenko
Copy link
Contributor

Description

Motivation

Workflow

  1. ⚠️⚠️ Create your PR as draft
  2. Follow the style guidelines of this project (See how to easily lint the code)
  3. Work on you PR until the CI passes (if something not related to your task is failing, you can ignore it)
  4. Mark it as ready for review

Once your PR is reviewed, you can merge it! ❤️

Reviewer checklist

  • Check what scenarios are modified. If needed, add the relevant label (run-parametric-scenario, run-profiling-scenario...). If this PR modifies any system-tests internal, then add the run-all-scenarios label (more info).
  • CI is green
    • If not, failing jobs are not related to this change (and you are 100% sure about this statement)
  • if any of build-some-image label is present
    1. is the image labl have been updated ?
    2. just before merging, locally build and push the image to hub.docker.com
  • if a scenario is added (or removed), add (or remove) it in system-test-dasboard nightly

@dianashevchenko dianashevchenko mentioned this pull request Nov 7, 2023
Merged
5 tasks
@dianashevchenko dianashevchenko changed the title Fixed the tests in accordance to OTel attributes remapping [GO]: OTel API operation naming: Unskipped / Fixed the tests in accordance to OTel attributes remapping Nov 8, 2023
@dianashevchenko dianashevchenko marked this pull request as ready for review November 10, 2023 17:00
@dianashevchenko dianashevchenko requested review from a team as code owners November 10, 2023 17:00
@katiehockman katiehockman mentioned this pull request Nov 10, 2023
Merged
5 tasks
robertomonteromiguel
robertomonteromiguel previously requested changes Nov 13, 2023
View reviewed changes
Copy link
Collaborator

@robertomonteromiguel robertomonteromiguel left a comment

Choose a reason for hiding this comment

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

There is a test failure on ci
There are conflicts with the main branch

tests/apm_tracing_e2e/test_otel.py Outdated
assert handler_span.get("parentID") == roundtrip_span.get("spanID")

# Assert the spans received from the backend!
spans = interfaces.backend.assert_request_spans_exist(self.req, query_filter="")
assert 3 == len(spans), _assert_msg(3, len(spans))


def _get_span(spans, span_name):
def _get_span(spans, span_name, resource_name):
Copy link
Collaborator

Choose a reason for hiding this comment

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

Perhaps, we can split this method in _get_span_by_name and _get_span_by_resource or something like this, in order to easily read the code. It's a proposal....

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi, I addressed the proposal, disregard the conflict with go mod files

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I also merged the main

@katiehockman katiehockman dismissed robertomonteromiguel’s stale review November 13, 2023 18:09

Comments have been addressed

@katiehockman
Copy link
Contributor

/merge

@dd-devflow
Copy link

dd-devflow bot commented Nov 13, 2023

🚂 MergeQueue

This merge request is not mergeable yet, because of pending checks/missing approvals. It will be added to the queue as soon as checks pass and/or get approvals.
Note: if you pushed new commits since the last approval, you may need additional approval.
You can remove it from the waiting list with /remove command.

you can cancel this operation by commenting your pull request with /merge -c!

@katiehockman
Copy link
Contributor

/remove

@dd-devflow
Copy link

dd-devflow bot commented Nov 13, 2023

🚂 Devflow: remove

@dd-devflow
Copy link

dd-devflow bot commented Nov 13, 2023

⚠️ MergeQueue

This merge request was unqueued

If you need support, contact us on slack #ci-interfaces!

@cbeauchesne
Copy link
Collaborator

@dianashevchenko please ping me if you need any help to solve the egg-chicken

@dianashevchenko dianashevchenko merged commit 4f35504 into main Nov 14, 2023
167 checks passed
@dianashevchenko dianashevchenko deleted the shevchenko/otel-remapping branch November 14, 2023 10:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cbeauchesne cbeauchesne cbeauchesne approved these changes

@katiehockman katiehockman katiehockman approved these changes

@robertomonteromiguel robertomonteromiguel Awaiting requested review from robertomonteromiguel

Assignees
No one assigned
Labels
mergequeue-status: removed run-open-telemetry-scenarios run-parametric-scenario
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@dianashevchenko @katiehockman @cbeauchesne @robertomonteromiguel