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

[Monitoring] [Pipeline Viewer] Remove obsolete code #20122

Conversation

justinkambic
Copy link
Contributor

@justinkambic justinkambic commented Jun 21, 2018

Motivation

As part of our revamp of the Pipeline Viewer, we have replaced a significant amount of visualization code. The old code may be useful in the future, but for the time being we have no plans to use it, so we are going to remove it from the active codebase and let it live in history for possible future reuse.

Closes #19736

Testing This PR

As the code we're removing is no longer in use, testing is a little obscure. The best procedure is making sure it passes CI, and running the view itself.

  1. Start up a Logstash pipeline with at least one branch.
  2. Navigate to X-Pack monitoring.
  3. Choose Logstash/Pipelines and open the view for your pipeline.
  4. Make sure all the features work.
    a. Click a plugin name to ensure DetailDrawer renders properly
    b. Collapse/expand your branch repeatedly
    c. Ensure no errors/warnings are logged to the dev console

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@justinkambic justinkambic added review and removed WIP Work in progress labels Jun 21, 2018
@justinkambic
Copy link
Contributor Author

@ycombinator @mattapperson should be good to review now, thanks

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

font-size: 13px;
stroke: none;
}

.lspvEdge {
Copy link
Contributor

Choose a reason for hiding this comment

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

Are the rest of the classes here still needed?

@ycombinator
Copy link
Contributor

I think its not as obvious but there's some obsolete code in the models folder as well, for example:

get svgClass() {
return `${super.svgClass} ${super.svgClass}Boolean ${super.svgClass}Boolean--${this.when}`;
}

@justinkambic
Copy link
Contributor Author

I think its not as obvious but there's some obsolete code in the models folder

I did find this but I was under the impression that it's not obsolete because it's used in the Graph model, no?

Are the rest of the classes here still needed?

Same question as above.

@ycombinator
Copy link
Contributor

I did find this but I was under the impression that it's not obsolete because it's used in the Graph model, no?

I think it depends. IMO, the only code that should be left in the Graph model classes after this PR is code that's needed by the conversion process to go from DAG -> config representation. Everything else should be obsolete, no?

@justinkambic
Copy link
Contributor Author

Everything else should be obsolete, no?

Yeah, that sounds good, I'll revisit this. 👍

@justinkambic justinkambic force-pushed the monitoring/pipeline-viewer-obsolete-code branch from 9a88661 to c6c0445 Compare June 22, 2018 22:24
@justinkambic
Copy link
Contributor Author

@ycombinator should be good for another 👀

@ycombinator
Copy link
Contributor

This is looking good. Found one more area that you can clean up:

I think this object key can go, including the methods used to generate it's value:

display: this.truncateStringForDisplay(this.id, this.displaySubtitleMaxLength)

In fact, once you remove the display key, the object will have just one key left, complete, so you could probably just remove the object wrapper altogether.

There's a similar area in the IfVertex class as well.

And remember the corresponding unit test code as well.

@justinkambic
Copy link
Contributor Author

@ycombinator e8fa994 should address the display prop

Copy link
Contributor

@ycombinator ycombinator left a comment

Choose a reason for hiding this comment

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

LGTM pending green CI. Great to see all the old code being removed from the codebase ❤️.

@elasticmachine
Copy link
Contributor

💔 Build Failed

@justinkambic justinkambic force-pushed the monitoring/pipeline-viewer-obsolete-code branch from 141176e to 36f369e Compare June 25, 2018 16:53
@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@justinkambic
Copy link
Contributor Author

@ycombinator can you please have a 👀 at 36f369e

If that LGTY then I think we are good to merge

Copy link
Contributor

@ycombinator ycombinator left a comment

Choose a reason for hiding this comment

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

LGTM after changes made to make CI go green.

@justinkambic justinkambic merged commit 94d99d6 into elastic:master Jun 26, 2018
justinkambic added a commit that referenced this pull request Jun 26, 2018
* Remove PipelineViewer component, replace with Config Viewer as index export.

* Remove tests for obsolete component.

* Remove obsolete code.

* Remove obsolete CSS.

* Remove old SVG class for graph edges.

* Remove more graph rendering code.

* Remove obsolete properties from graph classes.

* Remove unused constants.

* Remove obsolete keys from subtitle props.

* Fix broken unit tests.
@justinkambic
Copy link
Contributor Author

Backported to:
6.4.0/6.x 15667e5

@justinkambic justinkambic deleted the monitoring/pipeline-viewer-obsolete-code branch June 26, 2018 15:00
mattapperson pushed a commit that referenced this pull request Jun 28, 2018
* Remove PipelineViewer component, replace with Config Viewer as index export.

* Remove tests for obsolete component.

* Remove obsolete code.

* Remove obsolete CSS.

* Remove old SVG class for graph edges.

* Remove more graph rendering code.

* Remove obsolete properties from graph classes.

* Remove unused constants.

* Remove obsolete keys from subtitle props.

* Fix broken unit tests.
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.

3 participants