gracefully shutdown reporter & add agent-test-tool based e2e case #9

tisonkun · 2022-01-10T03:06:21Z

Signed-off-by: tison wander4096@gmail.com

Signed-off-by: tison <wander4096@gmail.com>

codecov-commenter · 2022-01-10T03:09:48Z

Codecov Report

Merging #9 (b21e2f4) into master (1465d9f) will decrease coverage by 3.44%.
The diff coverage is 7.69%.

@@            Coverage Diff             @@
##           master       #9      +/-   ##
==========================================
- Coverage   88.60%   85.15%   -3.45%     
==========================================
  Files           9        9              
  Lines         272      283      +11     
==========================================
  Hits          241      241              
- Misses         31       42      +11

Impacted Files	Coverage Δ
src/context/trace_context.rs	`69.81% <0.00%> (-6.48%)`	⬇️
src/reporter/grpc.rs	`0.00% <0.00%> (ø)`
src/context/propagation/encoder.rs	`100.00% <100.00%> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 1465d9f...b21e2f4. Read the comment docs.

tisonkun · 2022-01-10T03:26:56Z

I noticed that we may not use mpsc::channel at all.

Currently, the reporter flushes per context on the streaming rpc. May you provide other languages' reporter implementation as references and I can work for a proper API.

wu-sheng · 2022-01-10T03:33:51Z

Hi, if you want self-contained, this is another example. As a client, the official way to test is set up e2e, which this lib is only working as a client.
Self-contained is tricky because without SkyWalking OAP running, this client could send tracing w/ grpc means nothing.

wu-sheng · 2022-01-10T03:35:48Z

This is how the test thing works, https://github.com/SkyAPM/cpp2sky/blob/main/.github/workflows/main.yml

wu-sheng · 2022-01-10T03:41:07Z

I would like to recommend you to add an e2e test case for this repo, which has more value from our experiences.

Shikugawa · 2022-01-10T05:02:08Z

@tisonkun Thank you for awesome contribution! If you are interested in e2e framework, you should see here (This is my experimental work of e2e, but it is not ready for production). It may help you.

Shikugawa

Sorry, I'm not sure how situation we should use self-contained reporters. Could you show me the usecase for it?

Shikugawa · 2022-01-10T05:05:53Z

src/reporter/grpc.rs

+                    }
+                }
+            }
+            rx.close();
            while let Some(message) = rx.recv().await {


It may not be required anymore.

No. If a select chooses shutdown first, there can be outstanding messages to be processed. We close the rx so that there is no more inflight message, but the outstanding ones should be processed.

See also https://docs.rs/tokio/1.15.0/tokio/sync/mpsc/struct.Receiver.html#method.close for example.

examples/simple_trace_report.rs

tisonkun · 2022-01-10T06:50:51Z

I would like to recommend you to add an e2e test case for this repo, which has more value from our experiences.

That makes sense. I'm confused about how to run the example properly and failed to find a resource to set up the environment.

Thanks for your information. Let me dig it out.

wu-sheng · 2022-01-10T06:54:49Z

That makes sense. I'm confused about how to run the example properly and failed to find a resource to set up the environment.

Because you need a backend and UI, https://skywalking.apache.org/docs/main/v8.9.1/readme/.
About the self-hosted, it is not 100% meaningless, but mostly, it just could print the segment values.

Shikugawa · 2022-01-10T07:46:29Z

I would like to recommend you to add an e2e test case for this repo, which has more value from our experiences.

That makes sense. I'm confused about how to run the example properly and failed to find a resource to set up the environment.

Thanks for your information. Let me dig it out.

We preapred simple docker-compose envs here.
Also, your change will break e2e code here.

tisonkun · 2022-01-10T09:27:06Z

Also, your change will break e2e code here.

All checks have passed in CI. How do I verify this sentence?

tisonkun · 2022-01-10T09:29:01Z

@wu-sheng It seems the example can leave as is (without starting a server) and I can verify the graceful shutdown logic by fixing current e2e test.

wu-sheng · 2022-01-10T09:48:17Z

@tisonkun E2E has a very specific meaning in the SkyWalking context.

There are 2 kinds of E2E tests.

E2E with SkyWalking backend. (I think this isn't @Shikugawa 's suggestion, just mention it here to make the context clear)
You could learn this tool, which is an established sub-project for a docker-compose/KinD(k8s) env testing to verify how components work with each other because the SkyWalking ecosystem is super large and complex.
Plugin tests with agent-test-tool, which is a mock OAP server to collect and print data to a YAML for you. Here is how it is used in cpp2sky.

You should follow (2) more for the rust plugin. This test is working as rust-a -> rust-b, and both of them are sending trace data to mock-collector. Then verifying the data from mock-collector with the expected data file you provided.

tisonkun · 2022-01-10T15:14:11Z

@wu-sheng @Shikugawa I can try to fix and verify e2e failure locally, but it seems our CI doesn't check the e2e test. Shall we add it also?

wu-sheng · 2022-01-10T15:15:20Z

Yes, e2e is a new case.

Signed-off-by: tison <wander4096@gmail.com>

tisonkun · 2022-01-13T14:54:40Z

It seems that depends_on doesn't take effect and consumer/producer runs before collector and panic...I don't know how to process then. If I manually start service parts one by one, the test can pass.

kezhenxu94 · 2022-01-13T15:03:20Z

It seems that depends_on doesn't take effect and consumer/producer runs before collector and panic...I don't know how to process then. If I manually start service parts one by one, the test can pass.

depends_on should take effects, what is out of expectation is that consumer/provider tries to do something before OAP is ready. Please use

depends_on:
  oap:
    condition: service_healthy

And add a health check for OAP.

NOTE: version 3.7 of docker compose file might not accept this syntax but I know 3.8 and 2.1 can.

wu-sheng · 2022-01-13T15:03:23Z

I have seen you added a health check. This is important.

Signed-off-by: tison <wander4096@gmail.com>

Adding healthCheck causes hang without explicit reason. Given that the consumer started quickly, I tend to revert the healthCheck for now. Signed-off-by: tison <wander4096@gmail.com>

tisonkun · 2022-01-13T16:05:04Z

Finally I hack through it.

I think this is ready for review, ~~but I still confuse why bbda7cf causes hanging forever :(~~ EDIT: I use a slim container which doesn't have a curl installed >_<

Signed-off-by: tison <wander4096@gmail.com>

wu-sheng · 2022-01-14T00:40:11Z

.github/workflows/e2e.yml

+          pip3 install --upgrade pip
+          pip3 install setuptools
+          pip3 install -r requirements.txt
+          python3 tests/e2e/run_e2e.py --expected_file=tests/e2e/data/expected_context.yaml --max_retry_times=3 --target_path=/ping


I can't find the tests/e2e/data/expected_context.yaml. How does this test pass? Could you check the logs?
Is this a run_e2e.py bug or something?

It's already in the codebase here: https://github.com/apache/skywalking-rust/blob/1465d9fc6811dcb009e75f5c1dd2d5b1ef7a52a9/tests/e2e/data/expected_context.yaml

I don't add a new test case but according to your suggestion #9 (comment), enable the existing but not in regression e2e test - it's a plugin test based on agent-test-tool.

wu-sheng · 2022-01-14T02:25:36Z

@Shikugawa Please check the codes.

wu-sheng · 2022-01-14T02:32:00Z

@tisonkun Please rename the title to make the changes clear.

tisonkun · 2022-01-14T02:49:11Z

@wu-sheng updated. Notify me if it's better to split into two PRs.

wu-sheng · 2022-01-14T03:10:53Z

I am fine that these two are in one PR, as this repo is a very early stage.

But I hope you could recheck the test coverage

src/context/trace_context.rs | 69.81% <0.00%> (-6.48%)

I am not sure whether e2e testing could be counted in Rust, but it can be in the Java.

tisonkun · 2022-01-14T03:17:57Z

@wu-sheng I checked the report and don't think it's a real regression:

"Regressions" on impl fmt:Debug. It's meaningless to test a standard lib implementation.
"Regressions" on shutdown/sender. The e2e test should cover it.

Shikugawa

Great. Thank you for changing!

wu-sheng · 2022-01-14T15:10:08Z

I got a notification that e2e of merged commit has failed in 9s after e2e began.
It makes me feel the health check seems not working well.
I rerun the test, and will see whether it could pass.

tisonkun · 2022-01-15T03:46:48Z

@wu-sheng @Shikugawa according to the failure report, it seems the received data is unstable (not stably matches the expected data).

https://github.com/apache/skywalking-rust/runs/4818192129?check_suite_focus=true

diff list: 
- meterItems: []
  segmentItems:
  - segmentSize: 1
    segments:
-   - segmentId: '184682932856352416001309273098852514966'
+   - segmentId: not null
      spans:
      - componentId: 11000
-       endTime: 1642171583
+       endTime: gt 0
        isError: false
+       operationId: 0
        operationName: /pong
        parentSpanId: 1
        peer: consumer:8082
        skipAnalysis: false
        spanId: 2
        spanLayer: Http
        spanType: Exit
-       startTime: 1642171583
+       startTime: gt 0
      - componentId: 11000
-       endTime: 1642171583
+       endTime: gt 0
        isError: false
+       operationId: 0
        operationName: /ping
        parentSpanId: 0
        peer: ''
        skipAnalysis: false
        spanId: 1
        spanLayer: Http
        spanType: Entry
-       startTime: 1642171583
+       startTime: gt 0
    serviceName: producer
  - segmentSize: 1
    segments:
-   - segmentId: '251652345953849008728622634953354684403'
+   - segmentId: not null
      spans:
      - componentId: 11000
-       endTime: 1642171583
+       endTime: gt 0
        isError: false
+       operationId: 0
        operationName: /pong
        parentSpanId: 0
        peer: ''
        refs:
        - networkAddress: consumer:8082
          parentEndpoint: /pong
          parentService: producer
          parentServiceInstance: node_0
          parentSpanId: 2
-         parentTraceSegmentId: '184682932856352416001309273098852514966'
+         parentTraceSegmentId: not null
          refType: CrossProcess
-         traceId: '280108941388649570187429178111613482643'
+         traceId: not null
        skipAnalysis: false
        spanId: 1
        spanLayer: Http
        spanType: Entry
-       startTime: 1642171583
+       startTime: gt 0
    serviceName: consumer
Traceback (most recent call last):
  File "tests/e2e/run_e2e.py", line 60, in <module>
    assert res.status_code == 200
AssertionError

wu-sheng · 2022-01-15T05:17:16Z

I think the key point is AssertionError, which seems the observed services are not available.

self-contained example & gracefully shutdown reporter

ee8b736

Signed-off-by: tison <wander4096@gmail.com>

tisonkun force-pushed the trace-report-example branch from e8dd804 to ee8b736 Compare January 10, 2022 03:06

tokio-stream can be a dev-dependencies

89848ac

Signed-off-by: tison <wander4096@gmail.com>

Shikugawa requested changes Jan 10, 2022

View reviewed changes

tisonkun changed the title ~~self-contained example & gracefully shutdown reporter~~ gracefully shutdown reporter Jan 10, 2022

tisonkun added 2 commits January 10, 2022 23:31

revert example should not start a server

393c3b8

Signed-off-by: tison <wander4096@gmail.com>

adapt e2e test logic

b8fc551

Signed-off-by: tison <wander4096@gmail.com>

tisonkun force-pushed the trace-report-example branch from bc855b3 to b8fc551 Compare January 10, 2022 15:34

tisonkun marked this pull request as draft January 11, 2022 03:09

tisonkun force-pushed the trace-report-example branch 4 times, most recently from 11bc092 to 7f46110 Compare January 13, 2022 06:46

wu-sheng added the testing label Jan 13, 2022

tisonkun force-pushed the trace-report-example branch 2 times, most recently from 515a14a to 716bdb4 Compare January 13, 2022 13:52

tisonkun force-pushed the trace-report-example branch from 1240d59 to 54cfc17 Compare January 13, 2022 14:27

enable e2e test

9b20bb7

Signed-off-by: tison <wander4096@gmail.com>

tisonkun force-pushed the trace-report-example branch from 54cfc17 to 9b20bb7 Compare January 13, 2022 14:49

add healthcheck

86241d4

Signed-off-by: tison <wander4096@gmail.com>

tisonkun force-pushed the trace-report-example branch from d32013f to 86241d4 Compare January 13, 2022 15:14

tisonkun added 3 commits January 13, 2022 23:17

e2e main supports healthCheck

a62a35e

Signed-off-by: tison <wander4096@gmail.com>

reconcil docker-compose file

7bcd3a4

Signed-off-by: tison <wander4096@gmail.com>

revrt e2e main healthCheck

bbda7cf

Adding healthCheck causes hang without explicit reason. Given that the consumer started quickly, I tend to revert the healthCheck for now. Signed-off-by: tison <wander4096@gmail.com>

tisonkun marked this pull request as ready for review January 13, 2022 16:04

e2e main healthCheck using image with curl

b21e2f4

Signed-off-by: tison <wander4096@gmail.com>

tisonkun requested a review from Shikugawa January 13, 2022 16:36

wu-sheng reviewed Jan 14, 2022

View reviewed changes

wu-sheng added this to the 0.1.0 milestone Jan 14, 2022

tisonkun changed the title ~~gracefully shutdown reporter~~ gracefully shutdown reporter & add agent-test-tool based e2e case Jan 14, 2022

Shikugawa approved these changes Jan 14, 2022

View reviewed changes

wu-sheng merged commit f4743a9 into apache:master Jan 14, 2022

tisonkun deleted the trace-report-example branch January 14, 2022 14:59

tisonkun mentioned this pull request Jan 21, 2022

harden e2e test by adding healthCheck #10

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

gracefully shutdown reporter & add agent-test-tool based e2e case #9

gracefully shutdown reporter & add agent-test-tool based e2e case #9

tisonkun commented Jan 10, 2022

codecov-commenter commented Jan 10, 2022 •

edited

Loading

tisonkun commented Jan 10, 2022

wu-sheng commented Jan 10, 2022

wu-sheng commented Jan 10, 2022

wu-sheng commented Jan 10, 2022

Shikugawa commented Jan 10, 2022 •

edited

Loading

Shikugawa left a comment

Shikugawa Jan 10, 2022

tisonkun Jan 10, 2022

tisonkun commented Jan 10, 2022

wu-sheng commented Jan 10, 2022

Shikugawa commented Jan 10, 2022

tisonkun commented Jan 10, 2022

tisonkun commented Jan 10, 2022 •

edited

Loading

wu-sheng commented Jan 10, 2022

tisonkun commented Jan 10, 2022

wu-sheng commented Jan 10, 2022

tisonkun commented Jan 13, 2022

kezhenxu94 commented Jan 13, 2022 •

edited

Loading

wu-sheng commented Jan 13, 2022

tisonkun commented Jan 13, 2022 •

edited

Loading

wu-sheng Jan 14, 2022

tisonkun Jan 14, 2022

wu-sheng commented Jan 14, 2022

wu-sheng commented Jan 14, 2022

tisonkun commented Jan 14, 2022

wu-sheng commented Jan 14, 2022

tisonkun commented Jan 14, 2022 •

edited

Loading

Shikugawa left a comment

wu-sheng commented Jan 14, 2022

tisonkun commented Jan 15, 2022

wu-sheng commented Jan 15, 2022

gracefully shutdown reporter & add agent-test-tool based e2e case #9

gracefully shutdown reporter & add agent-test-tool based e2e case #9

Conversation

tisonkun commented Jan 10, 2022

codecov-commenter commented Jan 10, 2022 • edited Loading

Codecov Report

tisonkun commented Jan 10, 2022

wu-sheng commented Jan 10, 2022

wu-sheng commented Jan 10, 2022

wu-sheng commented Jan 10, 2022

Shikugawa commented Jan 10, 2022 • edited Loading

Shikugawa left a comment

Choose a reason for hiding this comment

Shikugawa Jan 10, 2022

Choose a reason for hiding this comment

tisonkun Jan 10, 2022

Choose a reason for hiding this comment

tisonkun commented Jan 10, 2022

wu-sheng commented Jan 10, 2022

Shikugawa commented Jan 10, 2022

tisonkun commented Jan 10, 2022

tisonkun commented Jan 10, 2022 • edited Loading

wu-sheng commented Jan 10, 2022

tisonkun commented Jan 10, 2022

wu-sheng commented Jan 10, 2022

tisonkun commented Jan 13, 2022

kezhenxu94 commented Jan 13, 2022 • edited Loading

wu-sheng commented Jan 13, 2022

tisonkun commented Jan 13, 2022 • edited Loading

wu-sheng Jan 14, 2022

Choose a reason for hiding this comment

tisonkun Jan 14, 2022

Choose a reason for hiding this comment

wu-sheng commented Jan 14, 2022

wu-sheng commented Jan 14, 2022

tisonkun commented Jan 14, 2022

wu-sheng commented Jan 14, 2022

tisonkun commented Jan 14, 2022 • edited Loading

Shikugawa left a comment

Choose a reason for hiding this comment

wu-sheng commented Jan 14, 2022

tisonkun commented Jan 15, 2022

wu-sheng commented Jan 15, 2022

codecov-commenter commented Jan 10, 2022 •

edited

Loading

Shikugawa commented Jan 10, 2022 •

edited

Loading

tisonkun commented Jan 10, 2022 •

edited

Loading

kezhenxu94 commented Jan 13, 2022 •

edited

Loading

tisonkun commented Jan 13, 2022 •

edited

Loading

tisonkun commented Jan 14, 2022 •

edited

Loading