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

WIP: Add changes to fix the span spilling over #1414

Closed
wants to merge 5 commits into from
Closed

WIP: Add changes to fix the span spilling over #1414

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
d9c2db9
Add changes to fix the span spilling over
ankur22 Sep 4, 2024
6c20c25
Fix url attribute on navigate and navigation span
ankur22 Sep 6, 2024
347c0e7
Remove the navigate span
ankur22 Sep 6, 2024
2b41885
Add comment on setAttribute of url on span
ankur22 Sep 6, 2024
cc53f31
Add comment on navCaught
ankur22 Sep 6, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 3 additions & 2 deletions common/frame_manager.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -586,9 +586,9 @@ func (m *FrameManager) NavigateFrame(frame *Frame, url string, parsedOpts *Frame
fid = frame.ID()
furl = frame.URL()
)
m.logger.Debugf("FrameManager:NavigateFrame",
m.logger.Infof("FrameManager:NavigateFrame",
"fmid:%d fid:%v furl:%s url:%s", fmid, fid, furl, url)
defer m.logger.Debugf("FrameManager:NavigateFrame:return",
defer m.logger.Infof("FrameManager:NavigateFrame:return",
"fmid:%d fid:%v furl:%s url:%s", fmid, fid, furl, url)

timeoutCtx, timeoutCancelFn := context.WithTimeout(m.ctx, parsedOpts.Timeout)
Expand Down Expand Up @@ -648,6 +648,7 @@ func (m *FrameManager) NavigateFrame(frame *Frame, url string, parsedOpts *Frame
return nil, fmt.Errorf("navigating to %q: %w", url, err)
}

// TODO: How does this affect spans?
if newDocumentID == "" {
// It's a navigation within the same document (e.g. via anchor links or
// the History API), so don't wait for a response nor any lifecycle
Expand Down
59 changes: 50 additions & 9 deletions common/frame_session.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -78,6 +78,12 @@
// Keep a reference to the main frame span so we can end it
// when FrameSession.ctx is Done
mainFrameSpan trace.Span
// This is set to false by default. When a goto is called it is set to true.
// The true is set when navigateFrame is called. It is reset back to false in
// onFrameNavigated where it will not create a new navigation span since one
// was created in navigateFrame. We need to start a navigation span earlier
// when a goto is called.
navCaught bool
}

// NewFrameSession initializes and returns a new FrameSession.
Expand Down Expand Up @@ -237,6 +243,11 @@
// If there is an active span for main frame,
// end it before exiting so it can be flushed
if fs.mainFrameSpan != nil {
// The url needs to be added here instead of at the start of the span
// because at the start of the span we don't know the correct url for
// the page we're navigating to. At the end of the span we do have this
// information.
fs.mainFrameSpan.SetAttributes(attribute.String("navigation.url", fs.manager.MainFrame().URL()))
fs.mainFrameSpan.End()
fs.mainFrameSpan = nil
}
Expand Down Expand Up @@ -267,6 +278,8 @@
fs.onFrameDetached(ev.FrameID, ev.Reason)
case *cdppage.EventFrameNavigated:
const initial = false
fs.logger.Infof("FrameSession:initEvents:go:EventFrameNavigated",
"sid:%v tid:%v", fs.session.ID(), fs.targetID)
fs.onFrameNavigated(ev.Frame, initial)
case *cdppage.EventFrameRequestedNavigation:
fs.onFrameRequestedNavigation(ev)
Expand Down Expand Up @@ -393,7 +406,7 @@
}

func (fs *FrameSession) initFrameTree() error {
fs.logger.Debugf("NewFrameSession:initFrameTree",
fs.logger.Infof("NewFrameSession:initFrameTree",
"sid:%v tid:%v", fs.session.ID(), fs.targetID)

action := cdppage.Enable()
Expand Down Expand Up @@ -595,7 +608,7 @@
}

func (fs *FrameSession) handleFrameTree(frameTree *cdppage.FrameTree, initialFrame bool) {
fs.logger.Debugf("FrameSession:handleFrameTree",
fs.logger.Infof("FrameSession:handleFrameTree",
"fid:%v sid:%v tid:%v", frameTree.Frame.ID, fs.session.ID(), fs.targetID)

if frameTree.Frame.ParentID != "" {
Expand All @@ -611,7 +624,7 @@
}

func (fs *FrameSession) navigateFrame(frame *Frame, url, referrer string) (string, error) {
fs.logger.Debugf("FrameSession:navigateFrame",
fs.logger.Infof("FrameSession:navigateFrame",
"sid:%v fid:%s tid:%v url:%q referrer:%q",
fs.session.ID(), frame.ID(), fs.targetID, url, referrer)

Expand Down Expand Up @@ -770,7 +783,7 @@
}

func (fs *FrameSession) onFrameNavigated(frame *cdp.Frame, initial bool) {
fs.logger.Debugf("FrameSession:onFrameNavigated",
fs.logger.Infof("FrameSession:onFrameNavigated",
"sid:%v tid:%v fid:%v",
fs.session.ID(), fs.targetID, frame.ID)

Expand All @@ -782,19 +795,41 @@
frame.URL+frame.URLFragment, err)
}

// We only want to run the next step if we've not already caught a
// navigation. navCaught will only really be true when a new page is
// created and chromium navigates to about:blank. Subsequent navigations
// that occur on the same page will perform the navigation span work
// in onFrameStartedLoading.
if fs.navCaught {
fs.navCaught = false
return
}

fs.doNavigationSpanStuff(frame.ParentID == "", frame.URL, frame.ID)
}

func (fs *FrameSession) doNavigationSpanStuff(isMainFrame bool, url string, id cdp.FrameID) {

Check warning on line 811 in common/frame_session.go

View workflow job for this annotation

GitHub Actions / lint 

unused-parameter: parameter 'url' seems to be unused, consider removing or renaming it as _ (revive)
// Trace navigation only for the main frame.
// TODO: How will this affect sub frames such as iframes?
if isMainFrame := frame.ParentID == ""; !isMainFrame {
if !isMainFrame {
return
}

if fs.mainFrameSpan != nil {
// The url needs to be added here instead of at the start of the span
// because at the start of the span we don't know the correct url for
// the page we're navigating to. At the end of the span we do have this
// information.
fs.mainFrameSpan.SetAttributes(attribute.String("navigation.url", fs.manager.MainFrame().URL()))
fs.mainFrameSpan.End()
}
_, fs.mainFrameSpan = TraceNavigation(
fs.ctx, fs.targetID.String(), trace.WithAttributes(attribute.String("navigation.url", frame.URL)),
fs.ctx, fs.targetID.String(),
)

var (
spanID = fs.mainFrameSpan.SpanContext().SpanID().String()
newFrame, ok = fs.manager.getFrameByID(frame.ID)
newFrame, ok = fs.manager.getFrameByID(id)
)

// Only set the k6SpanId reference if it's a new frame.
Expand Down Expand Up @@ -827,7 +862,7 @@
}

func (fs *FrameSession) onFrameRequestedNavigation(event *cdppage.EventFrameRequestedNavigation) {
fs.logger.Debugf("FrameSession:onFrameRequestedNavigation",
fs.logger.Infof("FrameSession:onFrameRequestedNavigation",
"sid:%v tid:%v fid:%v url:%q",
fs.session.ID(), fs.targetID, event.FrameID, event.URL)

Expand All @@ -840,10 +875,16 @@
}

func (fs *FrameSession) onFrameStartedLoading(frameID cdp.FrameID) {
fs.logger.Debugf("FrameSession:onFrameStartedLoading",
fs.logger.Infof("FrameSession:onFrameStartedLoading",
"sid:%v tid:%v fid:%v",
fs.session.ID(), fs.targetID, frameID)

frame, ok := fs.manager.getFrameByID(frameID)
if !fs.navCaught && ok {
fs.navCaught = true
fs.doNavigationSpanStuff(frame.page.frameManager.MainFrame() == frame, frame.URL(), frame.id)
}

fs.manager.frameLoadingStarted(frameID)
}

Expand Down
Loading