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

Sidecar collector rename #4903

Merged
merged 6 commits into from
Jul 19, 2018
Merged

Sidecar collector rename #4903

merged 6 commits into from
Jul 19, 2018

Conversation

mariussturm
Copy link
Contributor

Related to Graylog2/collector-sidecar#262
Fixes Graylog2/collector-sidecar#255

Description

After merging Graylog2/collector-sidecar#262 the Sidecar will only report the Collector-ID in the corresponding status response. So we have to change the response model as well as the frontend to resolve the ID to an actual collector name on the status page.

Copy link
Contributor

@edmundoa edmundoa left a comment

Choose a reason for hiding this comment

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

Apart from the inline comments, the changes didn't update the status information in the collectors administration page:

const result = sidecar.node_details.status.collectors.find(c => c.name === collector.name);

@@ -38,8 +40,13 @@ class SidecarStatusPage extends React.Component {
SidecarsActions.getSidecar(this.props.params.sidecarId).then(sidecar => this.setState({ sidecar }));
};

reloadCollectors() {
Copy link
Contributor

Choose a reason for hiding this comment

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

As this is an ES6 class, this should use an arrow function, to ensure this is properly bound, even if the function calling it is not autobound by React.

From https://reactjs.org/docs/react-without-es6.html#autobinding:

If you’d rather play it safe, you have a few options:

  • Bind methods in the constructor.
  • Use arrow functions, e.g. onClick={(e) => this.handleClick(e)}.
  • Keep using createReactClass.

@@ -9,6 +9,7 @@ import SidecarStatusFileList from './SidecarStatusFileList';
const SidecarStatus = createReactClass({
propTypes: {
sidecar: PropTypes.object.isRequired,
collectors: PropTypes.array.isRequired,
Copy link
Contributor

Choose a reason for hiding this comment

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

Going to a sidecar details page, I'm seeing the following in my JS console:

Warning: Failed prop type: The prop `collectors` is marked as required in `SidecarStatus`, but its value is `undefined`.
    in SidecarStatus (created by SidecarStatusPage)
    in SidecarStatusPage (created by ReactProxy)

@@ -25,6 +26,7 @@ class SidecarStatusPage extends React.Component {

componentDidMount() {
this.reloadSidecar();
this.reloadCollectors();
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we also need to automatically reload collectors, or is it fine to only load them on page load? I guess the second option is fine, but want to double-check :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Loading them once is good enough.

Copy link
Contributor

Choose a reason for hiding this comment

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

@bernd bernd added the sidecar label Jul 12, 2018
@mariussturm
Copy link
Contributor Author

I think the CollectorsAdministration page is not affected because we don't use the node_details.collectors list. Instead we do it exactly like in this PR, call CollectorsActions.all() and from that result we can use collector.name.

Copy link
Member

@bernd bernd left a comment

Choose a reason for hiding this comment

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

@mariussturm I am seeing the following issue:

The collector status is always unknown

image

In the sidecar overview the status of a collector is "running":

image

@bernd bernd dismissed edmundoa’s stale review July 19, 2018 05:27

Comments have been addresses.

@bernd bernd merged commit 36438c2 into master Jul 19, 2018
@bernd bernd deleted the sidecar-collector-rename branch July 19, 2018 05:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Renaming a collector backend results in duplicate processes
3 participants