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

Allows to modify the start time and end time properties of the SegmentSpan object. #437

Merged
merged 1 commit into from
Aug 19, 2021
Merged

Conversation

qq362220083
Copy link
Contributor

Why submit this pull request?

  • Bug fix
  • New feature provided
  • Improve performance

New feature

  • Allows to modify the start time and end time properties of the SegmentSpan object.

    In a specific scenario, I want to modify the start time and end time properties of the SegmentSpan object.
    For example, in the diagnostic information of StackExchange.Redis(SERedis), the start time and end time are determined by SERedis

}
}

public void SetTimes(DateTimeOffset startTime, DateTimeOffset endTime)
Copy link
Member

Choose a reason for hiding this comment

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

Why and when would you need to change manually? This is not a typical feature for SkyWalking.

Copy link
Member

Choose a reason for hiding this comment

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

For example, in the diagnostic information of StackExchange.Redis(SERedis), the start time and end time are determined by SERedis

I have read this, but it is not clear.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The collection method of SERedis is to collect all commands and time objects under the current link at one time.

like this: https://github.com/StackExchange/StackExchange.Redis/blob/main/src/StackExchange.Redis/Profiling/ProfiledCommandEnumerable.cs

it contains the exact time of the beginning and end of the event, so I hope to modify the time of the SegmentSpan object.

Copy link
Member

@wu-sheng wu-sheng Aug 16, 2021

Choose a reason for hiding this comment

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

@liuhaoyang Your call. This seems very language related.

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 tried to implement the ISegmentContextFactory interface, but still cannot modify the SegmentContext and SegmentSpan object properties.

@wu-sheng wu-sheng requested review from liuhaoyang and a team August 16, 2021 14:37
@wu-sheng wu-sheng added this to the 1.4.0 milestone Aug 16, 2021
Copy link
Collaborator

@liuhaoyang liuhaoyang left a comment

Choose a reason for hiding this comment

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

My suggestion is that you add StartTime to the API of ISegmentContextFactory and modify the Finish function of SegmentSpan to pass in the EndtTime parameter.

public interface ISegmentContextFactory
{
   SegmentContext CreateEntrySegment(string operationName, ICarrier carrier,long stratTime = default(...));
   ...
}
public class SegmentSpan 
{
 ...
  public void Finish(long endTime = default(...))
  {
    ...
  }
}

@wu-sheng wu-sheng requested a review from liuhaoyang August 19, 2021 03:37
@qq362220083
Copy link
Contributor Author

My suggestion is that you add StartTime to the API of ISegmentContextFactory and modify the Finish function of SegmentSpan to pass in the EndtTime parameter.

public interface ISegmentContextFactory
{
   SegmentContext CreateEntrySegment(string operationName, ICarrier carrier,long stratTime = default(...));
   ...
}
public class SegmentSpan 
{
 ...
  public void Finish(long endTime = default(...))
  {
    ...
  }
}

Okay, I have modified and commit it as suggested.

@liuhaoyang liuhaoyang merged commit e62ad6d into SkyAPM:master Aug 19, 2021
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.

3 participants