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.

Sign up for GitHub

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

Introduce SegmentcontextScope and some Improve #193

Closed
wants to merge 4 commits into from
Closed

Introduce SegmentcontextScope and some Improve #193

wants to merge 4 commits into from

Conversation

caozhiyuan
Copy link
Member

Please answer these questions before submitting pull request

  • Why submit this pull request?

  • Bug fix

  • New feature provided

  • Improve performance

  • Related issues

Bug fix

New feature or improvement

  • Describe the details and related test reports.
  • Introduce SegmentcontextScope (need test a lot)
  • StringOrIntValueHelpers ParseStringOrIntValue Just Use SubString
  • SkyApm.Sample.AspNet project change to PackageReference

@caozhiyuan
* Introduce SegmentContextScope
0074f98 
* SegmentReference NetworkAddress Use parentSegmentContext.Span.Peer
* StringOrIntValueHelpers ParseStringOrIntValue Just Use SubString
* SkyApm.Sample.AspNet project change to PackageReference
@caozhiyuan
Merge remote-tracking branch 'upstream/master' into segmentcontext_scope
0d3d116
@caozhiyuan caozhiyuan added bug Something isn't working enhancement New feature or request labels Apr 11, 2019
@caozhiyuan caozhiyuan requested a review from liuhaoyang April 11, 2019 11:13
@caozhiyuan
Copy link
Member Author

SegmentContextAccessor be removed , https://github.com/linkinshi/SkyAPM-dotnet-mysql need update to lastest.

@caozhiyuan caozhiyuan changed the title Introduce SegmentcontextScope and some performance Improve [WIP]Introduce SegmentcontextScope and some performance Improve Apr 11, 2019
@caozhiyuan caozhiyuan changed the title [WIP]Introduce SegmentcontextScope and some performance Improve [WIP]Introduce SegmentcontextScope and some Improve Apr 11, 2019
@wu-sheng
Copy link
Member

Why Mysql plugin is in another repo?

@liuhaoyang liuhaoyang added this to the 0.9.0 milestone Apr 11, 2019
@liuhaoyang
Copy link
Collaborator

In previous versions, I also used SegmentContextAsyncLocalScope to manage the current Segment. But in asynchronous scenarios, it raises some bug .. #102 #72

@liuhaoyang
Copy link
Collaborator

Why Mysql plugin is in another repo?

Mysql.Data lib is under the GPL license. Uh. I am worried that it will lead to incompatibility of the license.

@wu-sheng
Copy link
Member

If it is compiled level, it is fine. SkyWalking has mysql plugin as default. But if your binary requires that, that is another story.

In my mind, plugin only works after user use it, so, in that case, you don't include/depend mysql, user does.

@caozhiyuan
Copy link
Member Author

@liuhaoyang https://github.com/caozhiyuan/SkyAPM-dotnet/blob/0.7.0/src/SkyWalking.Core/Context/TracingContext.cs#L48
no scope , this stack is not safe in multi-thread.

@caozhiyuan
Copy link
Member Author

caozhiyuan commented Apr 11, 2019 •
edited
Loading

i want to merge spans in one segmentcontext, but it need some time . now , one span one segmentcontext, but it simple.

@wu-sheng
Copy link
Member

For multiple thread case, I highly recommend you ref SkyWalking javaagent did for vertx. That is 100% async framework, and it works perfectly with our new API.

@liuhaoyang
Copy link
Collaborator

@caozhiyuan I have discussed with sheng about the problem of segment. Letting each span as a segment is my compromise in the C# Async syntax.

@liuhaoyang
Copy link
Collaborator

@liuhaoyang https://github.com/caozhiyuan/SkyAPM-dotnet/blob/0.7.0/src/SkyWalking.Core/Context/TracingContext.cs#L48
no scope , this stack is not safe in multi-thread.

Each request is a scope...

@caozhiyuan
Copy link
Member Author

caozhiyuan commented Apr 11, 2019 •
edited
Loading

@liuhaoyang run SkyApm.Sample.Frontend.Controllers http://localhost:5001/api/values/2 test it. it diff with previous versions.

every excutecontext has a new scope , previous versions same tracingContext instance.

@caozhiyuan
Copy link
Member Author

@liuhaoyang any problem about SegmentContextScope ?

@caozhiyuan caozhiyuan changed the title [WIP]Introduce SegmentcontextScope and some Improve Introduce SegmentcontextScope and some Improve Apr 18, 2019
@caozhiyuan caozhiyuan closed this May 25, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@liuhaoyang liuhaoyang Awaiting requested review from liuhaoyang

Assignees
No one assigned
Labels
bug Something isn't working enhancement New feature or request
Projects
None yet
Milestone
0.9.0
Development

Successfully merging this pull request may close these issues.
3 participants
@caozhiyuan @wu-sheng @liuhaoyang