Extending the configuration server to provide a live and dirty state of the configuration

[mariusoe]

This change is 

[JonasKunz]

Haven't fully looked at teh UI, because I feel like my suggested change will cause a lot of changes.

Server-Stuff: I like it a lot!

Reviewed 34 of 55 files at r1.
Reviewable status: 34 of 55 files reviewed, 28 unresolved discussions (waiting on @JonasKunz and @mariusoe)

---

components/inspectit-ocelot-configurationserver/build.gradle, line 123 at r1 (raw file):

            "org.springframework.security:spring-security-ldap",
            'org.springframework.boot:spring-boot-starter-actuator',

Are these changes intentional?

---

components/inspectit-ocelot-configurationserver/src/main/java/rocks/inspectit/ocelot/file/FileManager.java, line 33 at r1 (raw file):

public class FileManager {

    public static final ReadWriteLock WORKING_DIRECTORY_LOCK = new ReentrantReadWriteLock();

DOn't make this public static final, use private final instead.
Then pass the individual locks (read or write or both) to created accessors.

---

components/inspectit-ocelot-configurationserver/src/main/java/rocks/inspectit/ocelot/file/FileManager.java, line 75 at r1 (raw file):

            return versioningManager.getWorkspaceDiff(includeContent);
        } catch (IOException | GitAPIException e) {
            log.error("error", e);

Better log message than error please :D

---

components/inspectit-ocelot-configurationserver/src/main/java/rocks/inspectit/ocelot/file/FileManager.java, line 76 at r1 (raw file):

        } catch (IOException | GitAPIException e) {
            log.error("error", e);
            return null;

Instead of null, make the return type Optional<WorkspaceDiff> or throw a (Runtime)Exception.

---

components/inspectit-ocelot-configurationserver/src/main/java/rocks/inspectit/ocelot/file/accessor/git/RevisionAccess.java, line 91 at r1 (raw file):

        } catch (Exception e) {
            log.error("Could not read file {} from git repository", path, e);
            return Optional.empty();

Any reason for no throwing runtime exceptions here?
Same on the methods below
This is especially misleading for exists and isDirectory()

---

components/inspectit-ocelot-configurationserver/src/main/java/rocks/inspectit/ocelot/file/accessor/git/RevisionAccess.java, line 133 at r1 (raw file):

     * @throws IOException in case the repository cannot be read
     */
    private List<FileInfo> listFiles(String path, boolean recursive) throws IOException {

Isn't recursive always ture in our usages?
Same for collectFiles.

If this is the case, then just eleminate the non-recurisve case.

---

components/inspectit-ocelot-configurationserver/src/main/java/rocks/inspectit/ocelot/file/accessor/git/RevisionAccess.java, line 159 at r1 (raw file):

            }

            return collectFiles(treeWalk, recursive, skipNext);

add an if(!skipNext) treeWalk.next() here and remove skipNext from collectFiles.

---

components/inspectit-ocelot-configurationserver/src/main/java/rocks/inspectit/ocelot/file/accessor/git/RevisionAccess.java, line 181 at r1 (raw file):

        while (skipNext || treeWalk.next()) {

turn this into a do .. while and remove skipNext.
This means that the method will never invoke next() for the first iteration.
The caller then is responsible for doing so.

---

components/inspectit-ocelot-configurationserver/src/main/java/rocks/inspectit/ocelot/file/accessor/git/RevisionAccess.java, line 188 at r1 (raw file):

            FileInfo.FileInfoBuilder fileBuilder = FileInfo.builder().name(name);

            if (recursive && treeWalk.isSubtree()) {

This means directories will only be listed if recursive is true, which is wrong.
But as stated above, simply remove the non-recursive case.

---

components/inspectit-ocelot-configurationserver/src/main/java/rocks/inspectit/ocelot/file/accessor/workingdirectory/AutoCommitWorkingDirectoryProxy.java, line 23 at r1 (raw file):

    private VersioningManager versioningManager;

    public AutoCommitWorkingDirectoryProxy(WorkingDirectoryAccessor workingDirectoryAccessor, VersioningManager versioningManager) {

Add a Lock writeLock parameter to this constructor.
Then use this to do the locking below.

---

components/inspectit-ocelot-configurationserver/src/main/java/rocks/inspectit/ocelot/file/accessor/workingdirectory/AutoCommitWorkingDirectoryProxy.java, line 78 at r1 (raw file):

            clean();
            workingDirectoryAccessor.move(sourcePath, targetPath);
            commit();

Just noticed that this actually allows us to recognize "move" operations in the future!
(But not required now :D)

---

components/inspectit-ocelot-configurationserver/src/main/java/rocks/inspectit/ocelot/file/accessor/workingdirectory/WorkingDirectoryAccessor.java, line 70 at r1 (raw file):

    @Override
    protected Optional<byte[]> readFile(String path) {
        FileManager.WORKING_DIRECTORY_LOCK.readLock().lock();

Same as for the AutoCommitProxy:
Add a Lock writeLock and Lock readLock parameter to the constructor and use these for locking.

---

components/inspectit-ocelot-configurationserver/src/main/java/rocks/inspectit/ocelot/file/accessor/workingdirectory/WorkingDirectoryAccessor.java, line 78 at r1 (raw file):

            }

            try {

You can remove this try .. catch and isntead move the catch to the surrounding try ... finally

---

components/inspectit-ocelot-configurationserver/src/main/java/rocks/inspectit/ocelot/file/accessor/workingdirectory/WorkingDirectoryAccessor.java, line 98 at r1 (raw file):

            }

            try {

Same as above

---

components/inspectit-ocelot-configurationserver/src/main/java/rocks/inspectit/ocelot/file/versioning/VersioningManager.java, line 149 at r1 (raw file):

     * @param message the commit message to use
     */
    public synchronized void commit(String message) throws GitAPIException {

All commit methods do not state anything about the branch to which the commits happen right now.
Name them accordingly or at least state that they commit to the work branch.

In addition you should add a check that HEAD actually points to the work branch.

---

components/inspectit-ocelot-configurationserver/src/main/java/rocks/inspectit/ocelot/file/versioning/VersioningManager.java, line 162 at r1 (raw file):

     * @param allowAmend whether commits should be amended if possible (same user and below timeout)
     */
    private synchronized void commit(PersonIdent author, String message, boolean stageFiles, boolean allowAmend) throws GitAPIException {

split this intocommitStagedChanges and commitAllChanges instead of using stageFiles as parameter.

---

components/inspectit-ocelot-configurationserver/src/main/java/rocks/inspectit/ocelot/file/versioning/VersioningManager.java, line 168 at r1 (raw file):

        // lock the working directory
        FileManager.WORKING_DIRECTORY_LOCK.writeLock().lock();

I would suggest that this class does not need to take care of any locking (neither the synchronized, nor the lock).

Leave it to the user of this class to ensure that no concurrent accesses happen.
This would remove this complexity-burden from this class.

---

components/inspectit-ocelot-configurationserver/src/main/java/rocks/inspectit/ocelot/file/versioning/VersioningManager.java, line 184 at r1 (raw file):

     * Commits the staged files using the given author and message. Consecutive of the same user within {@link #amendTimeout}
     * milliseconds will be amended if specified. When used, ensure to lock the working directory, otherwise it may be
     * that you' re committing to the wrong branch.

As said above, just add a comment to the class stating that the caller needs to make sure no race-conditions / concurrent accesses happen.

---

components/inspectit-ocelot-configurationserver/src/main/java/rocks/inspectit/ocelot/file/versioning/VersioningManager.java, line 277 at r1 (raw file):

     * @return Returns the latest commit of the workspace branch.
     */
    public Optional<RevCommit> getLatestCommit() {

Remove this method. The one with the parameter is sufficient.

---

components/inspectit-ocelot-configurationserver/src/main/java/rocks/inspectit/ocelot/file/versioning/VersioningManager.java, line 349 at r1 (raw file):

     * difference (old and new content) will not be returned.
     */
    public WorkspaceDiff getWorkspaceDiff() throws IOException, GitAPIException {

Name this getWorkspaceDiffWithoutContent.

---

components/inspectit-ocelot-configurationserver/src/main/java/rocks/inspectit/ocelot/file/versioning/VersioningManager.java, line 471 at r1 (raw file):

        // lock the working directory
        FileManager.WORKING_DIRECTORY_LOCK.writeLock().lock();

Again, I would remove the locking responsibility from this class

---

components/inspectit-ocelot-configurationserver/src/main/java/rocks/inspectit/ocelot/file/versioning/VersioningManager.java, line 515 at r1 (raw file):

            // commit changes
            commit(getCurrentAuthor(), "Promoting configuration files", false, false);

Open a TODO issue: allow customizable commit messages for promotion

---

components/inspectit-ocelot-configurationserver/src/main/java/rocks/inspectit/ocelot/file/versioning/VersioningManager.java, line 525 at r1 (raw file):

            eventPublisher.publishEvent(new ConfigurationPromotionEvent(this));

            //TODO should we hard reset the repository in case an error occurs, thus we're in a clean state?

Shouldn't the checkout cleanly handle this?

---

components/inspectit-ocelot-configurationserver/src/main/java/rocks/inspectit/ocelot/file/versioning/model/SimpleDiffEntry.java, line 10 at r1 (raw file):

 * This class represents a single file diff including all information about it.
 */
@Data

I usually would prefer @Value over @Data, but I'm not sure if you used the mutability somewhere.

---

components/inspectit-ocelot-configurationserver/src/main/java/rocks/inspectit/ocelot/file/versioning/model/WorkspaceDiff.java, line 24 at r1 (raw file):

     */
    @JsonProperty("entries")
    private List<SimpleDiffEntry> diffEntries;

Why name the variable and the JSON-entry differently?
Why not just name both entries?

---

components/inspectit-ocelot-configurationserver/src/main/java/rocks/inspectit/ocelot/rest/configuration/PromotionController.java, line 36 at r1 (raw file):

    @ApiOperation(value = "Promote configurations", notes = "Promotes the specified configuration files.")
    @PostMapping(value = "configuration/promote")
    public ResponseEntity promoteConfiguration(@ApiParam("The definition that contains the information about which files to promote.") @RequestBody ConfigurationPromotion promotion) throws GitAPIException {

Missing access control

---

components/inspectit-ocelot-configurationserver/src/test/java/rocks/inspectit/ocelot/file/accessor/git/RevisionAccessIntTest.java, line 56 at r1 (raw file):

        revision = versioningManager.getLiveRevision();

        System.out.println("Test data in: " + tempDirectory.toString());

This is supposed to be here, right?

---

components/inspectit-ocelot-configurationserver-ui/src/components/dialogs/DialogContainer.js, line 8 at r1 (raw file):

 * Container element containing all dialogs.
 */
const DialogContainer = () => {

DO we really need a separate component for this...? :D

---

components/inspectit-ocelot-configurationserver-ui/src/components/dialogs/promotions/PromotionApprovalDialog.js, line 15 at r1 (raw file):

  const dispatch = useDispatch();

  const canCommit = useSelector((state) => state.authentication.permissions.commit);

In my opinion nothing regarding configuration promotion belongs into the redux-state.
Wehre else would this information be used except for within the promotion view?
Therefore I think everything should be component-state.

It is important that we keep the redux state as simple as possible in my opinion.

---

components/inspectit-ocelot-configurationserver-ui/src/components/dialogs/promotions/PromotionApprovalDialog.js, line 21 at r1 (raw file):

  if (!canCommit) {
    // dialog not needed
    return <></>;

Btw, never knew you could write empty tags in Order to get a simple container :D

[mariusoe]

Reviewable status: 34 of 55 files reviewed, 7 unresolved discussions (waiting on @JonasKunz and @mariusoe)

---

components/inspectit-ocelot-configurationserver/build.gradle, line 123 at r1 (raw file):

Previously, JonasKunz (Jonas Kunz) wrote…

Are these changes intentional?

No, but the spring components are nicely grouped together now :)

---

components/inspectit-ocelot-configurationserver/src/main/java/rocks/inspectit/ocelot/file/FileManager.java, line 33 at r1 (raw file):

Previously, JonasKunz (Jonas Kunz) wrote…

DOn't make this public static final, use private final instead.
Then pass the individual locks (read or write or both) to created accessors.

Done.

---

components/inspectit-ocelot-configurationserver/src/main/java/rocks/inspectit/ocelot/file/accessor/git/RevisionAccess.java, line 181 at r1 (raw file):

Previously, JonasKunz (Jonas Kunz) wrote…

turn this into a do .. while and remove skipNext.
This means that the method will never invoke next() for the first iteration.
The caller then is responsible for doing so.

That's much more elegant than my code :D

---

components/inspectit-ocelot-configurationserver/src/main/java/rocks/inspectit/ocelot/file/accessor/workingdirectory/AutoCommitWorkingDirectoryProxy.java, line 78 at r1 (raw file):

Previously, JonasKunz (Jonas Kunz) wrote…

Just noticed that this actually allows us to recognize "move" operations in the future!
(But not required now :D)

True. I'll open a ticket

---

components/inspectit-ocelot-configurationserver/src/main/java/rocks/inspectit/ocelot/file/accessor/workingdirectory/WorkingDirectoryAccessor.java, line 70 at r1 (raw file):

Previously, JonasKunz (Jonas Kunz) wrote…

Same as for the AutoCommitProxy:
Add a Lock writeLock and Lock readLock parameter to the constructor and use these for locking.

Imo we don't need two locks, but just a ReadWriteLock should be sufficient

---

components/inspectit-ocelot-configurationserver/src/main/java/rocks/inspectit/ocelot/file/versioning/VersioningManager.java, line 162 at r1 (raw file):

Previously, JonasKunz (Jonas Kunz) wrote…

split this intocommitStagedChanges and commitAllChanges instead of using stageFiles as parameter.

I just made a commitAllChanges which stages and commits the files. There is no other one, now, so staging has to be done "manually" if using the private commitFiles method.

---

components/inspectit-ocelot-configurationserver/src/main/java/rocks/inspectit/ocelot/file/versioning/model/SimpleDiffEntry.java, line 10 at r1 (raw file):

Previously, JonasKunz (Jonas Kunz) wrote…

I usually would prefer @Value over @Data, but I'm not sure if you used the mutability somewhere.

I'm modifying the values in the version manager, thus, need the data annotation.

---

components/inspectit-ocelot-configurationserver-ui/src/components/dialogs/DialogContainer.js, line 8 at r1 (raw file):

Previously, JonasKunz (Jonas Kunz) wrote…

DO we really need a separate component for this...? :D

I would say so :)
Makes it quite easy to add new dialogs because they are nicely grouped together in one place.

[JonasKunz]

Reviewed 14 of 14 files at r2.
Reviewable status: 35 of 56 files reviewed, 11 unresolved discussions (waiting on @JonasKunz and @mariusoe)

---

components/inspectit-ocelot-configurationserver/src/main/java/rocks/inspectit/ocelot/file/accessor/git/RevisionAccess.java, line 80 at r2 (raw file):

            if (treeWalk.isSubtree()) {
                throw new IllegalArgumentException("Target must be a file but found directory: " + path);

This won't be caught by AbstractFileAccessor.readConfigurationFile/readAgentMappings. Is this intentional?

---

components/inspectit-ocelot-configurationserver/src/main/java/rocks/inspectit/ocelot/file/accessor/git/RevisionAccess.java, line 127 at r2 (raw file):

     * @throws IOException in case the repository cannot be read
     */
    private List<FileInfo> listFiles2(String path) throws IOException {

listFiles2 - why the "2"?

---

components/inspectit-ocelot-configurationserver/src/main/java/rocks/inspectit/ocelot/file/accessor/git/RevisionAccess.java, line 139 at r2 (raw file):

                treeWalk = new TreeWalk(repository);
                treeWalk.addTree(tree);
                treeWalk.setRecursive(false);

According to your previous code skipNext was false in this case?
This means that the code now behaves differently?

To achieve the same behaviour, you would need to call treeWalk.next() here, right?

---

components/inspectit-ocelot-configurationserver/src/main/java/rocks/inspectit/ocelot/file/accessor/workingdirectory/AutoCommitWorkingDirectoryProxy.java, line 24 at r2 (raw file):

    private VersioningManager versioningManager;

    public AutoCommitWorkingDirectoryProxy(ReadWriteLock workingDirectoryLock, WorkingDirectoryAccessor workingDirectoryAccessor, VersioningManager versioningManager) {

You are jsut using the writeLock in this class, so it would avtually be sufficient to jsut accept a java.util.concurrent.locks.Lock here instead of a ReadWriteLock, but this is a matter of taste I guess.

---

components/inspectit-ocelot-configurationserver/src/main/java/rocks/inspectit/ocelot/file/accessor/workingdirectory/WorkingDirectoryAccessor.java, line 70 at r1 (raw file):

Previously, mariusoe (Marius Oehler) wrote…

Imo we don't need two locks, but just a ReadWriteLock should be sufficient

Again, amtter of taste :D With ReadWriteLock you force the Implementation, while for two separate Locks you leave this decision to the caller

---

components/inspectit-ocelot-configurationserver/src/main/java/rocks/inspectit/ocelot/file/accessor/workingdirectory/WorkingDirectoryAccessor.java, line 80 at r2 (raw file):

            if (Files.isDirectory(targetPath)) {
                throw new IllegalArgumentException("The specified '" + path + "' is not a file but directory.");

This also won't be caught by AbstractFileAccessor

---

components/inspectit-ocelot-configurationserver/src/main/java/rocks/inspectit/ocelot/file/versioning/VersioningManager.java, line 506 at r2 (raw file):

            // commit changes
            commitFiles(getCurrentAuthor(), "Promoting configuration files", false);

Here you want to commit files on the "live"-branch, but "commitFiles" states that it works on the working-directory branch?

---

components/inspectit-ocelot-configurationserver/src/main/java/rocks/inspectit/ocelot/rest/configuration/PromotionController.java, line 37 at r2 (raw file):

    }

    @Secured(UserRoleConfiguration.COMMIT_ACCESS_ROLE)

I think we should rename the role to "reviewer", but doesn't need to be done in this ticket

[mariusoe]

Reviewable status: 33 of 48 files reviewed, 3 unresolved discussions (waiting on @JonasKunz)

---

components/inspectit-ocelot-configurationserver/src/main/java/rocks/inspectit/ocelot/file/accessor/git/RevisionAccess.java, line 127 at r2 (raw file):

Previously, JonasKunz (Jonas Kunz) wrote…

listFiles2 - why the "2"?

Ups :)

---

components/inspectit-ocelot-configurationserver/src/main/java/rocks/inspectit/ocelot/file/accessor/git/RevisionAccess.java, line 139 at r2 (raw file):

Previously, JonasKunz (Jonas Kunz) wrote…

According to your previous code skipNext was false in this case?
This means that the code now behaves differently?

To achieve the same behaviour, you would need to call treeWalk.next() here, right?

Good catch

---

components/inspectit-ocelot-configurationserver-ui/src/components/dialogs/DialogContainer.js, line 8 at r1 (raw file):

Previously, mariusoe (Marius Oehler) wrote…

I would say so :)
Makes it quite easy to add new dialogs because they are nicely grouped together in one place.

Refactored this. Is acutally more diffucult and less flexible.

[codecov]

Codecov Report

Merging #790 into master will decrease coverage by 0.08%.
The diff coverage is 81.08%.

@@             Coverage Diff              @@
##             master     #790      +/-   ##
============================================
- Coverage     83.00%   82.93%   -0.08%     
- Complexity     1736     1760      +24     
============================================
  Files           174      178       +4     
  Lines          5366     5470     +104     
  Branches        650      658       +8     
============================================
+ Hits           4454     4536      +82     
- Misses          675      694      +19     
- Partials        237      240       +3     

Impacted Files	Coverage Δ	Complexity Δ
...t/ocelot/config/model/tracing/TracingSettings.java	100.00% <ø> (ø)	0.00 <0.00> (ø)
...lation/log/adapters/JBossLogmanagerMDCAdapter.java	66.67% <66.67%> (ø)	2.00 <2.00> (?)
...nstrumentation/context/ContextPropagationUtil.java	84.73% <70.37%> (-3.00%)	31.00 <3.00> (+5.00)	⬇️
...rumentation/context/propagation/DatadogFormat.java	77.14% <77.14%> (ø)	7.00 <7.00> (?)
...trumentation/context/PropagationFormatManager.java	80.00% <80.00%> (ø)	3.00 <3.00> (?)
...ocelot/config/model/tracing/PropagationFormat.java	100.00% <100.00%> (ø)	1.00 <1.00> (?)
...ore/instrumentation/correlation/log/MDCAccess.java	87.80% <100.00%> (+3.93%)	16.00 <3.00> (+3.00)
...ion/correlation/log/adapters/Log4J1MDCAdapter.java	80.00% <100.00%> (+2.22%)	4.00 <1.00> (+1.00)
...ion/correlation/log/adapters/Log4J2MDCAdapter.java	80.00% <100.00%> (+2.22%)	4.00 <1.00> (+1.00)
...tion/correlation/log/adapters/Slf4jMDCAdapter.java	80.00% <100.00%> (+2.22%)	4.00 <1.00> (+1.00)
... and 10 more

[JonasKunz]

Reviewed 2 of 55 files at r1, 35 of 35 files at r3.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @JonasKunz and @mariusoe)

---

components/inspectit-ocelot-configurationserver-ui/src/components/views/promotion/PromotionToolbar.js, line 48 at r3 (raw file):

          <Button
            disabled={loading}
            tooltip="Reload Configurations"

Wrong tooltip

---

components/inspectit-ocelot-configurationserver-ui/src/components/views/promotion/dialogs/PromotionConflictDialog.js, line 32 at r3 (raw file):

    >
      <p>One or more configurations have been promoted in the meantime.</p>
      <p>Your current state is out of sync, thus, cannot be promoted in order to negative side effects.</p>

"cannot be promoted in order to negative side effects."

[mariusoe]

Reviewed 20 of 55 files at r1, 4 of 14 files at r2, 24 of 35 files at r3, 2 of 2 files at r4.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @JonasKunz)

[mariusoe]

Reviewed 1 of 35 files at r3, 1 of 1 files at r5.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @JonasKunz)

[JonasKunz]

Reviewed 2 of 2 files at r4, 1 of 1 files at r5.
Reviewable status: complete! all files reviewed, all discussions resolved