Skip to content

Commit

Permalink
feat(rum-react): use correct path when route is path array (#800)
Browse files Browse the repository at this point in the history
* feat(rum-react): use correct path when route is path array

* feat: move logic in apmroute

* chore: address review and add cb test

* fix: change name only if defined
  • Loading branch information
vigneshshanmugam authored Jun 15, 2020
1 parent cc56d85 commit 18ee0bf
Show file tree
Hide file tree
Showing 6 changed files with 170 additions and 66 deletions.
139 changes: 83 additions & 56 deletions package-lock.json

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

4 changes: 2 additions & 2 deletions package.json
Original file line number Diff line number Diff line change
Expand Up @@ -149,8 +149,8 @@
"puppeteer": "^1.20.0",
"react": "^16.2.0",
"react-dom": "^16.2.0",
"react-router": "^4.2.0",
"react-router-dom": "^4.2.2",
"react-router": "^5.2.0",
"react-router-dom": "^5.2.0",
"regenerator-runtime": "^0.13.3",
"rimraf": "^3.0.0",
"serve-index": "^1.9.1",
Expand Down
26 changes: 24 additions & 2 deletions packages/rum-react/src/get-apm-route.js
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,19 @@ import React from 'react'
import { Route } from 'react-router-dom'
import { getWithTransaction } from './get-with-transaction'

/**
* If the path/name is given as array, use the computed path
* to get the current transaction name
*/
function getTransactionName(name, props) {
const { match = {} } = props

if (Array.isArray(name) && match.path) {
return match.path
}
return name
}

function getApmRoute(apm) {
const withTransaction = getWithTransaction(apm)

Expand All @@ -48,8 +61,17 @@ function getApmRoute(apm) {
*/
if (initial || pathChanged) {
return {
apmComponent: withTransaction(path, 'route-change')(component),
path
path,
apmComponent: withTransaction(
path,
'route-change',
(transaction, props) => {
if (transaction) {
const name = getTransactionName(path, props)
name && (transaction.name = name)
}
}
)(component)
}
}
return null
Expand Down
11 changes: 7 additions & 4 deletions packages/rum-react/src/get-with-transaction.js
Original file line number Diff line number Diff line change
Expand Up @@ -46,7 +46,7 @@ function isReactClassComponent(Component) {
* - As a decorator: `@withTransaction('name','route-change')`
*/
function getWithTransaction(apm) {
return function withTransaction(name, type) {
return function withTransaction(name, type, callback = () => {}) {
return function(Component) {
const configService = apm.serviceFactory.getService('ConfigService')
if (!configService.isActive()) {
Expand Down Expand Up @@ -89,12 +89,14 @@ function getWithTransaction(apm) {
* want this piece of code to run on every render instead we want to
* start the transaction only on component mounting
*/
const [transaction] = React.useState(() =>
apm.startTransaction(name, type, {
const [transaction] = React.useState(() => {
const tr = apm.startTransaction(name, type, {
managed: true,
canReuse: true
})
)
callback(tr, props)
return tr
})

/**
* React guarantees the parent component effects are run after the child components effects
Expand Down Expand Up @@ -131,6 +133,7 @@ function getWithTransaction(apm) {
managed: true,
canReuse: true
})
callback(this.transaction, props)
}

componentDidMount() {
Expand Down
37 changes: 37 additions & 0 deletions packages/rum-react/test/specs/get-apm-route.spec.js
Original file line number Diff line number Diff line change
Expand Up @@ -105,6 +105,43 @@ describe('ApmRoute', function() {
)
})

it('should work correctly with path array in props', function() {
const ApmRoute = getApmRoute(apmBase)

const transactionService = serviceFactory.getService('TransactionService')
const dummyTr = {
name: 'test',
detectFinish: () => {}
}
spyOn(transactionService, 'startTransaction').and.returnValue(dummyTr)

const Home = () => 'home'
class Topics extends React.Component {
render() {
return 'Topics'
}
}

const rendered = mount(
<Router initialEntries={['/', '/topic1', '/topic2']}>
<ApmRoute path={['/']} component={Home} />
<ApmRoute path={['/topic1', '/topic2']} component={Topics} />
</Router>
)
expect(dummyTr.name).toEqual('/')
expect(rendered.text()).toBe('home')
const history = rendered.find(Home).props().history

history.push({ pathname: '/topic2' })
expect(dummyTr.name).toEqual('/topic2')
expect(transactionService.startTransaction).toHaveBeenCalledTimes(2)
/**
* Should not create transaction as component is not rerendered
*/
history.push({ pathname: '/topic1' })
expect(transactionService.startTransaction).toHaveBeenCalledTimes(2)
})

it('should not trigger full rerender on query change', function() {
const ApmRoute = getApmRoute(apmBase)
const transactionService = serviceFactory.getService('TransactionService')
Expand Down
19 changes: 17 additions & 2 deletions packages/rum-react/test/specs/get-with-transaction.spec.js
Original file line number Diff line number Diff line change
Expand Up @@ -34,14 +34,15 @@ import { ApmBase } from '@elastic/apm-rum'
import { createServiceFactory, afterFrame } from '@elastic/apm-rum-core'
import { getGlobalConfig } from '../../../../dev-utils/test-config'

function TestComponent(apm) {
function TestComponent(apm, cb) {
const withTransaction = getWithTransaction(apm)
function Component(props) {
return <h1>Testing, {props.name}</h1>
}
const WrappedComponent = withTransaction(
'test-transaction',
'test-type'
'test-type',
cb
)(Component)
expect(typeof WrappedComponent).toBe('function')
const wrapped = mount(<WrappedComponent name="withTransaction" />)
Expand Down Expand Up @@ -145,6 +146,20 @@ describe('withTransaction', function() {
expect(wrapper.text()).toBe('Testing, new-props')
})

it('should accept callback function for withTransaction', () => {
const transactionService = serviceFactory.getService('TransactionService')
const labels = {
foo: 'bar'
}
TestComponent(apmBase, tr => {
tr.name = 'name-changed'
tr.addLabels(labels)
})
const transaction = transactionService.getCurrentTransaction()
expect(transaction.name).toBe('name-changed')
expect(transaction.context.tags).toEqual(labels)
})

it('should end transaction when component unmounts', done => {
const transactionService = serviceFactory.getService('TransactionService')
const detectFinishSpy = jasmine.createSpy('detectFinish')
Expand Down

0 comments on commit 18ee0bf

Please sign in to comment.